-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add master password #1963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a master-password migration flow, global unlock/expiry and external signing APIs, registry/metadata enhancements (knownTypes/signedExtensions), many new/changed hooks and UI components, Lottie animation support, storage/constant changes, and bumps @polkadot/* deps to ^16.4.8. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI (SignArea / SignUsingPassword)
participant Hooks as Hooks (build extrinsicPayload)
participant BG as Background (Extension)
participant Node as Node (RPC)
UI->>Hooks: Build extrinsicPayload & signerPayload
UI->>BG: pri(signing.getSignature, signerPayload)
BG->>BG: apply registry (knownTypes, signedExtensions) & produce signature
BG-->>UI: return signature (Hex) or null
UI->>Node: submit extrinsic with payloadHex + signature
Node-->>UI: Broadcast → InBlock → Finalized (TxResult)
sequenceDiagram
autonumber
actor User as User
participant Login as Login Screen
participant Check as useCheckMasterPassword
participant Nav as Router
participant Migr as MigratePasswords
participant BG as Background
User->>Login: Enter password
Login->>Check: validate local accounts
alt accounts need migration
Login->>Nav: navigate('/migratePasswords')
User->>Migr: set master password
loop per local account
Migr->>BG: pri(accounts.changePassword / accounts.changePasswordAll)
BG-->>Migr: boolean success/fail
end
Migr->>BG: pri(lockExtension)
Migr->>Nav: navigate('/')
else no migration
Login->>BG: pri(accounts.unlockAll, password, cacheTime)
BG-->>Login: success / failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Potential areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/extension-polkagate/src/components/HasProxyIndicator.tsx (2)
5-5: Inconsistent genesis hash constant sources.The code mixes genesis hash constants from two different sources:
WESTEND_GENESIS_HASHandKUSAMA_GENESIS_HASHfrom../util/constants(line 11)POLKADOT_GENESISfrom@polkagate/apps-config(line 5)This inconsistency is error-prone. All three chains should use constants from the same source to ensure they represent the same data format and are maintained consistently.
Apply this diff to use constants consistently from
@polkagate/apps-config:-import { KUSAMA_GENESIS, POLKADOT_GENESIS, WESTEND_GENESIS } from '@polkagate/apps-config'; +import { KUSAMA_GENESIS_HASH, POLKADOT_GENESIS_HASH, WESTEND_GENESIS_HASH } from '@polkagate/apps-config'; import { Data } from 'iconsax-react'; import React, { useCallback, useEffect, useState } from 'react'; import { useNavigate } from 'react-router-dom'; import { useProxies, useSelectedAccount, useTranslation } from '../hooks'; -import { KUSAMA_GENESIS_HASH, WESTEND_GENESIS_HASH } from '../util/constants'; import MyTooltip from './MyTooltip';Then update the hook calls:
- const westendProxies = useProxies(WESTEND_GENESIS_HASH, account?.address); - const kusamaProxies = useProxies(KUSAMA_GENESIS_HASH, account?.address); - const polkadotProxies = useProxies(POLKADOT_GENESIS, account?.address); + const westendProxies = useProxies(WESTEND_GENESIS_HASH, account?.address); + const kusamaProxies = useProxies(KUSAMA_GENESIS_HASH, account?.address); + const polkadotProxies = useProxies(POLKADOT_GENESIS_HASH, account?.address);And update line 68:
- const genesisHash = hasProxy?.polkadot ? POLKADOT_GENESIS : hasProxy?.kusama ? KUSAMA_GENESIS : WESTEND_GENESIS; + const genesisHash = hasProxy?.polkadot ? POLKADOT_GENESIS_HASH : hasProxy?.kusama ? KUSAMA_GENESIS_HASH : WESTEND_GENESIS_HASH;Also applies to: 11-11, 24-26
37-61: Fix state mutation and stale flag issues.The
useEffecthas two critical problems:
Direct state mutation: The code mutates the previous state object (
pre.westend = true) and returns the same reference. React requires immutable updates—mutating state can prevent re-renders and violates React principles.Stale flags: Once a proxy flag is set to
true, it never resets if proxies later become empty or undefined, leading to incorrect UI state.Apply this diff to fix both issues:
useEffect((): void => { - if (westendProxies?.length) { - setHasProxy((pre) => { - pre.westend = true; - - return pre; - }); - } - - if (kusamaProxies?.length) { - setHasProxy((pre) => { - pre.kusama = true; - - return pre; - }); - } - - if (polkadotProxies?.length) { - setHasProxy((pre) => { - pre.polkadot = true; - - return pre; - }); - } + setHasProxy({ + westend: !!westendProxies?.length, + kusama: !!kusamaProxies?.length, + polkadot: !!polkadotProxies?.length + }); }, [kusamaProxies, polkadotProxies, westendProxies]);packages/extension-polkagate/src/hooks/useProxies.ts (1)
28-46: Stale data when proxiedAddress changes.The
getProxiescallback only fetches if!proxies, which means once proxies are fetched for one address, changingproxiedAddresswon't trigger a refetch. The hook will return stale proxies from the previous address.Example scenario:
- Hook called with
proxiedAddress = 'address_A'→ fetches and sets proxies for A- Component updates
proxiedAddress = 'address_B'→useEffectruns, callsgetProxies(), but condition!proxiesis false, so no refetch occurs- Hook returns proxies for address A instead of B
Apply this diff to reset proxies when the address changes:
+ useEffect(() => { + // Reset proxies when address changes to trigger refetch + setProxies(undefined); + setProxiesWithAvailability(undefined); + }, [proxiedAddress]); + const getProxies = useCallback(() => { if (!proxies && api) {packages/extension-polkagate/src/components/SignArea3.tsx (1)
102-137: Fix SignerPayload construction to use correct fields and types
- method: use
preparedTransaction.method.toHex()instead ofapi.createType('Call', preparedTransaction)- replace
runtimeVersionwith
specVersion: api.runtimeVersion.specVersiontransactionVersion: api.runtimeVersion.transactionVersion- use
api.registry.signedExtensionsinstead of the hardcoded extension list- wrap
tipinapi.registry.createType('Compact<Balance>', 0)and ensurenonceisCompact<Index>packages/extension-polkagate/src/components/SignUsingPassword.tsx (1)
87-121: Allow retrial when signature retrieval failsIf
getSignaturereturnsnull(account locked) or throws, we sethasErrortotrueand never clear it. Because the buttons are disabled whenhasErroris true, the user gets stuck with no way to try again after unlocking the account. Instead of latching the error flag, clear it before the next attempt (or gate the disable condition on something retryable) and surface the error via existing flows so the user can recover.
🧹 Nitpick comments (8)
packages/extension-polkagate/src/components/ShortAddress.tsx (1)
29-29: Consider using a functional updater for consistency.For consistency with the improvement made on line 43, the
decreaseCharactersCountcallback should also use a functional state updater. This prevents potential stale closure issues and aligns with React best practices.Apply this diff:
- const decreaseCharactersCount = useCallback(() => clipped && setCharactersCount(charactersCount - 1), [charactersCount, clipped]); + const decreaseCharactersCount = useCallback(() => clipped && setCharactersCount((prev) => prev - 1), [clipped]);packages/extension-polkagate/src/hooks/useProxies.ts (1)
40-40: Improve error handling beyond console logging.Using
console.errorprovides no user feedback or recovery mechanism. Consider propagating errors to the caller or logging to a monitoring service.Example approach:
- }).catch(console.error); + }).catch((error) => { + console.error('Failed to fetch proxies:', error); + // Optionally: set an error state that callers can check + // setProxies([]); + });packages/extension-polkagate/src/components/PasswordInput.tsx (2)
66-66: Fix spacing in type annotation.The type annotation has a missing space after the colon.
Apply this diff:
- value?:string; + value?: string;
80-82: Remove unnecessary nullish coalescing operator.The
?? nullin the onChange handler is confusing and serves no purpose.HTMLInputElement.valueis always a string (nevernullorundefined), so the nullish coalescing operator will never trigger. The code should simply passvaluedirectly.Apply this diff to simplify the onChange handler:
const onChange = useCallback(({ target: { value } }: React.ChangeEvent<HTMLInputElement>) => { - onPassChange(value ?? null); + onPassChange(value); }, [onPassChange]);packages/extension-polkagate/src/fullscreen/onboarding/Bread.tsx (1)
77-83: Align the cursor with the disabled state.While MIGRATE_PASSWORDS reuses the disabled color, the stack still shows a pointer cursor, suggesting it's clickable. Switch the cursor to
defaultfor both WELCOME and MIGRATE states to match the navigation guard.packages/extension-polkagate/src/components/MatchPasswordField.tsx (1)
90-99: Promise handling: prefer await or drop try/catchtry/catch here won’t catch async rejections; you already chain .catch. Either await or remove try/catch.
Suggested:
- const handleConfirm = useCallback(() => { + const handleConfirm = useCallback(async () => { if (passwordStatus === PASSWORD_STATUS.MATCHED && onSetPassword) { - try { - onSetPassword()?.catch(console.error); - } catch (error) { - console.error('Error setting password:', error); - } + try { + await onSetPassword?.(); + } catch (error) { + console.error('Error setting password:', error); + } } - }, [onSetPassword, passwordStatus]); + }, [onSetPassword, passwordStatus]);packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
14-24: Redundant isExternal checkYou already filter non-external accounts; remove the inner isExternal guard.
- const results = await Promise.all(nonExternalAccount.map(async (account) => { - const { address, isExternal } = account; - - if (isExternal) { - return; - } + const results = await Promise.all(nonExternalAccount.map(async (account) => { + const { address } = account;packages/extension-polkagate/src/util/workers/utils/metadataFromApi.ts (1)
28-40: metaCalls size and schema alignment
- Using full runtimeMetadata increases payload size; calls-only is lighter and commonly used for injection metadata.
- metadataVersion is not part of MetadataDef (in this PR). Either add it to the interface or omit.
Suggested:
- knownTypes: api.registry.knownTypes, - metaCalls: base64Encode(api.runtimeMetadata.toU8a()), - metadataVersion: api.runtimeMetadata.version, - signedExtensions: api.registry.signedExtensions, + knownTypes: api.registry.knownTypes, + // keep calls-only to minimize payload + metaCalls: base64Encode(api.runtimeMetadata.asCallsOnly.toU8a()), + signedExtensions: api.registry.signedExtensions, @@ - types: getSpecTypes(api.registry, chainName, api.runtimeVersion.specName, api.runtimeVersion.specVersion) as Record<string, string | Record<string, string>>, - userExtensions: (api.registry as any).userExtensions + types: getSpecTypes(api.registry, chainName, api.runtimeVersion.specName, api.runtimeVersion.specVersion) as Record<string, string | Record<string, string>>, + userExtensions: (api.registry as any).userExtensionsIf you need metadataVersion, please add it as an optional field to MetadataDef for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
package.json(3 hunks)packages/extension-base/package.json(1 hunks)packages/extension-base/src/background/handlers/Extension.ts(6 hunks)packages/extension-base/src/background/types.ts(4 hunks)packages/extension-base/src/defaults.ts(1 hunks)packages/extension-chains/src/bundle.ts(2 hunks)packages/extension-inject/package.json(1 hunks)packages/extension-inject/src/types.ts(2 hunks)packages/extension-polkagate/src/class/endpointManager.ts(1 hunks)packages/extension-polkagate/src/class/endpointManager2.ts(1 hunks)packages/extension-polkagate/src/components/HasProxyIndicator.tsx(2 hunks)packages/extension-polkagate/src/components/Loading.tsx(2 hunks)packages/extension-polkagate/src/components/MatchPasswordField.tsx(3 hunks)packages/extension-polkagate/src/components/PasswordInput.tsx(5 hunks)packages/extension-polkagate/src/components/ShortAddress.tsx(1 hunks)packages/extension-polkagate/src/components/SignArea3.tsx(8 hunks)packages/extension-polkagate/src/components/SignUsingPassword.tsx(7 hunks)packages/extension-polkagate/src/context/ExtensionLockContext.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/onboarding/Bread.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx(1 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts(1 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsFlying.tsx(1 hunks)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts(1 hunks)packages/extension-polkagate/src/hooks/useLoginInfo.tsx(1 hunks)packages/extension-polkagate/src/hooks/useProxies.ts(1 hunks)packages/extension-polkagate/src/messaging.ts(3 hunks)packages/extension-polkagate/src/partials/Review.tsx(1 hunks)packages/extension-polkagate/src/popup/home/ManageHome.tsx(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/types.ts(1 hunks)packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)packages/extension-polkagate/src/util/chains.ts(2 hunks)packages/extension-polkagate/src/util/constants.ts(2 hunks)packages/extension-polkagate/src/util/workers/utils/metadataFromApi.ts(1 hunks)packages/extension-ui/src/Popup/routes/featuresRoutes.ts(2 hunks)packages/extension/package.json(1 hunks)packages/extension/public/locales/en/translation.json(1 hunks)packages/extension/src/packageInfo.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
packages/extension/src/packageInfo.ts (6)
packages/extension-chains/src/bundle.ts (1)
packageInfo(11-11)packages/extension-base/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-compat-metamask/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-chains/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-dapp/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-inject/src/packageInfo.ts (1)
packageInfo(6-6)
packages/extension-polkagate/src/popup/home/ManageHome.tsx (1)
packages/extension-polkagate/src/popup/home/index.tsx (1)
Home(25-90)
packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (2)
packages/extension-polkagate/src/components/Loading.tsx (1)
getStorage(38-48)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (2)
packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/messaging.ts (1)
areAccountsLocksExpired(100-102)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (4)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-base/src/background/handlers/Extension.ts (1)
accountsValidate(218-226)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)packages/extension-polkagate/src/components/contexts.tsx (1)
AccountContext(34-34)
packages/extension-base/src/background/types.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
packages/extension-polkagate/src/hooks/useLoginInfo.tsx (5)
packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/components/Loading.tsx (2)
getStorage(38-48)setStorage(71-84)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)packages/extension-polkagate/src/popup/passwordManagement/types.ts (2)
LoginInfo(15-21)LOGIN_STATUS(4-11)
packages/extension-polkagate/src/components/Loading.tsx (3)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
useIsFlying(10-30)packages/extension-polkagate/src/hooks/useLoginInfo.tsx (1)
useLoginInfo(13-64)packages/extension-polkagate/src/util/constants.ts (1)
ALLOWED_URL_ON_RESET_PASSWORD(230-230)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (10)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(29-45)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(18-23)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(96-98)windowOpen(259-261)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(71-84)updateStorage(19-36)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
LOGIN_STATUS(4-11)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (7)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(29-45)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(91-109)lockExtension(134-162)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(71-84)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)
packages/extension-polkagate/src/hooks/useProxies.ts (2)
packages/extension-polkagate/src/util/types.ts (2)
ProxyTypes(582-582)Proxy(584-589)packages/extension-polkagate/src/hooks/useChainInfo.ts (1)
useChainInfo(42-73)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(93-150)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)
packages/extension-polkagate/src/util/chains.ts (1)
replacements/interfaces.js (2)
selectableNetworks(150-150)selectableNetworks(150-150)
packages/extension-chains/src/bundle.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/messaging.ts (2)
packages/extension-base/src/background/handlers/Extension.ts (3)
accountsValidate(218-226)accountsChangePassword(91-109)getSignature(506-525)packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/components/SignUsingPassword.tsx (3)
packages/extension-base/src/background/handlers/Extension.ts (1)
getSignature(506-525)packages/extension-polkagate/src/messaging.ts (1)
getSignature(92-94)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
submitExtrinsic(152-204)
packages/extension-polkagate/src/components/HasProxyIndicator.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/constants.ts (2)
WESTEND_GENESIS_HASH(60-60)KUSAMA_GENESIS_HASH(59-59)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
[error] 52-52: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 112-112: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (23)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (2)
11-11: LGTM! Import consolidation improves code cleanliness.The import statement consolidation removes duplication and maintains the same functionality. The hook is correctly destructured and all values are used appropriately in the component.
18-18: LGTM! Formatting adjustment.The space before the closing parenthesis is a formatting preference with no functional impact.
packages/extension-polkagate/src/components/ShortAddress.tsx (1)
42-44: Good improvement: functional state updater.The change to use
setCharactersCount((prev) => prev + 1)instead of referencingcharactersCountdirectly is a best practice for state updates that depend on the previous state value. This ensures the latest state is always used, especially in concurrent rendering scenarios.packages/extension-polkagate/src/hooks/useProxies.ts (1)
21-22: LGTM! Clean API simplification.The refactor to accept
genesisHashand derive the API internally viauseChainInfosimplifies caller code and maintains a clear separation of concerns.packages/extension-polkagate/src/components/PasswordInput.tsx (3)
112-112: Good accessibility improvement!Adding
tabIndex={-1}to the password visibility toggle button prevents it from being included in the keyboard tab order. This allows users to tab directly from the password input to the next focusable element, improving the keyboard navigation experience.
4-4: LGTM!The Grid wrapper replacement is a straightforward layout change that maintains the component's functionality.
Also applies to: 97-97, 150-150
66-66: Consistent uncontrolled usage confirmed. All PasswordInput instances omit the value prop (uncontrolled mode) consistently; no risk of switching modes.packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
17-29: LGTM! Clear documentation added.The JSDoc provides a clear description of the hook's behavior, return values, and usage example without altering functionality.
packages/extension-base/package.json (1)
24-24: LGTM! Consistent Polkadot JS dependency updates.The updates to
@polkadot/api,@polkadot/rpc-provider, and@polkadot/typesare aligned with the broader PR-wide upgrade to ^16.4.8. The verification requested inpackages/extension/package.jsonwill cover these dependencies as well.Also applies to: 31-32
packages/extension-polkagate/src/popup/home/ManageHome.tsx (1)
13-13: No functional changes detected.Despite the change marker, this line appears to contain the same logic as before. If this was only a formatting adjustment, no review is needed.
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
21-21: Cosmetic change: trailing newline added.This is a standard formatting convention with no functional impact.
packages/extension-inject/package.json (1)
21-23: LGTM! Consistent Polkadot JS dependency updates.These updates align with the broader PR-wide upgrade to ^16.4.8, maintaining consistency across packages.
packages/extension/src/packageInfo.ts (1)
6-6: LGTM! Version bump aligns with package.json.The version update from
1.12.1to1.13.1is consistent with the corresponding change inpackages/extension/package.json. As this is an auto-generated file, ensure the build process regenerates it correctly.packages/extension-polkagate/src/class/endpointManager2.ts (1)
6-6: Confirm ENDPOINT_TIMEOUT value remains unchanged.Please ensure the new
ENDPOINT_TIMEOUT = AUTO_LOCK_PERIOD_DEFAULT * 60 * 1000(15 minutes) matches the previousNO_PASS_PERIODvalue to maintain backward compatibility.packages/extension/package.json (1)
18-20: Validated @polkadot/api@^16.4.8 with no security advisories Confirmed the package version is published and no vulnerabilities were found. Safe to merge.packages/extension-polkagate/src/components/MatchPasswordField.tsx (2)
101-114: Grid swap LGTMSwitch to MUI Grid with sx={style} is fine.
4-4: LGTM: import updateImporting Grid from MUI matches the JSX change.
packages/extension-inject/src/types.ts (2)
9-9: Import RegisteredTypes LGTM
72-75: Extend MetadataDef LGTMOptional knownTypes and signedExtensions keep backward compat.
packages/extension-polkagate/src/partials/Review.tsx (1)
281-282: Prop passthrough LGTMwithCancel is correctly passed to SignArea3.
packages/extension-polkagate/src/hooks/index.ts (3)
47-47: Export useIsFlying LGTM
52-52: Export useIsPasswordMigrated LGTM
56-56: Export useLoginInfo LGTM
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
166-201: Extrinsics that fail now hang and incorrectly report successThis implementation never inspects
dispatchErrororsystem.ExtrinsicFailedevents, so any on-chain failure (e.g., insufficient balance, module error) is still reported assuccess: true. Additionally, if the node rejects or drops the extrinsic before it appears in a finalized block, the promise never resolves and the UI hangs indefinitely. The previoussendfunction (lines 93-150) correctly handles dispatch errors via the status callback. Switch toapi.rpc.author.submitAndWatchExtrinsicto monitor tx status and events, inspectresult.dispatchErrorto populatefailureTextand setsuccess: falseon failures, and always unsubscribe on terminal states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension-base/src/background/handlers/Extension.ts (2)
481-486: Return after rejecting on missing password to avoid decode with undefined.Currently, execution continues and pair.decodePkcs8(password) may throw with undefined.
// if the keyring pair is locked, the password is needed if (pair.isLocked && !password) { reject(new Error('Password needed to unlock the account')); + return false; }
491-496: Use the per-request registry when signing.Adopt createSigningRegistry for JSON payloads and avoid the global registry.
- if (isJsonPayload(payload)) { - this.handleRegistry(payload); - } - - const result = request.sign(registry, pair); + const reg = isJsonPayload(payload) + ? this.createSigningRegistry(payload) + : new TypeRegistry(); + + const result = request.sign(reg, pair);
♻️ Duplicate comments (2)
packages/extension-polkagate/src/components/Loading.tsx (1)
93-105: Still normalize the reset URL before allow-listingReset routes that carry query strings (
/forgot-password?address=...) or nested segments still miss the allow-list because we compare the raw hash viaincludes. The reset flow stays locked behind the login overlay, same bug as previously reported.Normalize the hash (strip query/fragment) and compare with
some/startsWithso reset URLs continue to work even with params.- const extensionUrl = window.location.hash.replace('#', ''); + const extensionUrl = window.location.hash.replace('#', ''); + const currentPath = extensionUrl.split(/[?#]/)[0]; const condition = isExtensionLocked || !children || isFlying; - const isResettingWallet = step === STEPS.SHOW_LOGIN && ALLOWED_URL_ON_RESET_PASSWORD.includes(extensionUrl); + const isResettingWallet = step === STEPS.SHOW_LOGIN && + ALLOWED_URL_ON_RESET_PASSWORD.some((allowedPath) => currentPath.startsWith(allowedPath));packages/extension-base/src/background/handlers/Extension.ts (1)
136-145: Lock loop: handle forgotten accounts and purge stale cache entries (duplicate of prior review).keyring.getPair(address) can throw for forgotten accounts, aborting the loop and leaving other accounts unlocked. Catch and delete the stale cache entry, then continue.
- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + + if (!pair.isLocked) { + pair.lock(); + } + + this.#cachedUnlocks[address] = 0; + } catch { + delete this.#cachedUnlocks[address]; + } + });
🧹 Nitpick comments (2)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx (1)
46-46: Remove or document redundantas voidon navigate calls.The codebase consistently annotates
navigate(…)withas void, but in React Router v6navigatealready returnsvoid. Unless a specific lint rule or TypeScript setting mandates these assertions, they’re unnecessary. Either drop them or add a comment/guideline explaining why they’re needed.packages/extension-base/src/background/handlers/Extension.ts (1)
407-437: Unlock‑all: clamp cacheTime and remove debug logs.
- console.log of accounts leaks PII in prod logs.
- cacheTime is unbounded; clamp to a safe max (e.g., PASSWORD_EXPIRY_MS) and validate > 0.
- console.log('get message to unlock accounts ...'); + // unlocking multiple accounts with provided password @@ - console.log('localAccounts:', localAccounts); + // local (non-external) accounts only @@ - this.#cachedUnlocks[address] = Date.now() + cacheTime; + const safeCache = + Number.isFinite(cacheTime) && cacheTime > 0 + ? Math.min(cacheTime, PASSWORD_EXPIRY_MS) + : PASSWORD_EXPIRY_MS; + this.#cachedUnlocks[address] = Date.now() + safeCache;Please confirm whether different per-account passwords are still supported post‑migration; if yes, consider short‑circuiting on the first decodePkcs8 failure and returning false instead of throwing so the caller can surface a friendly error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/extension-base/src/background/handlers/Extension.ts(7 hunks)packages/extension-polkagate/src/components/Loading.tsx(2 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(2 hunks)packages/extension-polkagate/src/util/defaultType.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/util/defaultType.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/components/Loading.tsx (3)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
useIsFlying(10-30)packages/extension-polkagate/src/hooks/useLoginInfo.tsx (1)
useLoginInfo(13-64)packages/extension-polkagate/src/util/constants.ts (1)
ALLOWED_URL_ON_RESET_PASSWORD(230-230)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Release
🔇 Additional comments (1)
packages/extension-base/src/background/handlers/Extension.ts (1)
439-441: Empty cache returns true — confirm semantics.areLocksExpired() returns true when cache is empty. If the intent is “no active unlocks ⇒ expired,” this is fine; otherwise, adjust to return false when there are no entries.
since login is mandatory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (1)
25-34: Strengthen validation forautoLockPeriod.The guard only checks for
undefined, which may not catch all invalid states:
- Incomplete validation: If
autoLockPeriodcan benull,0, or negative, these values would pass the check but could produce incorrect behavior.- Line 29 calculation risk:
Date.now() - autoLockPeriodwithautoLockPeriod = 0yields the current time (no lock), and negative values could set a future timestamp.- No user feedback: When
autoLockPeriod === undefined, the button silently does nothing.Consider strengthening the validation:
const onClick = useCallback((): void => { - if (autoLockPeriod === undefined) { + if (autoLockPeriod === undefined || autoLockPeriod === null || autoLockPeriod <= 0) { return; } updateStorage(STORAGE_KEY.LOGIN_INFO, { lastLoginTime: Date.now() - autoLockPeriod }).then(() => { setExtensionLock(true); navigate('/') as void; lockExtension().catch(console.error); }).catch(console.error); }, [autoLockPeriod, navigate, setExtensionLock]);
♻️ Duplicate comments (5)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
33-44: Guard against undefined accounts and reset state properly.This issue was previously flagged but remains unresolved. The current implementation has two problems:
- Line 40 can call
getAccountsNeedsMigrationwithundefinedaccounts (when context hasn't loaded yet), causing a runtime error when trying to filter.- Lines 34-36 return early when password is missing but don't reset the state, leaving stale migration data visible.
Apply this diff to fix both issues:
useEffect(() => { - if (!pass) { + if (!pass || !accounts?.length) { + setAccountsNeedMigration(undefined); return; } setAccountsNeedMigration(undefined); getAccountsNeedsMigration(accounts, pass).then((res) => { setAccountsNeedMigration(res); }).catch(console.error); // eslint-disable-next-line react-hooks/exhaustive-deps }, [accounts?.length, pass]);packages/extension-base/src/background/handlers/Extension.ts (4)
136-145: Guard against missing pairs and clean up stale cache entries.As previously noted,
keyring.getPair(address)throws when an account is forgotten but the cache entry remains. The thrown error aborts the loop, preventing other accounts from being locked.Apply the previously suggested fix:
- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + + if (!pair.isLocked) { + pair.lock(); + } + + this.#cachedUnlocks[address] = 0; + } catch (error) { + delete this.#cachedUnlocks[address]; + } + });
163-163: chrome.tabs.reload does not return a Promise.As previously flagged, calling
.catchonchrome.tabs.reloadwill throw at runtime since the Chrome API doesn't return a Promise.Apply the previously suggested fix:
- chrome.tabs.reload(id).catch(console.error); + withErrorLog(() => chrome.tabs.reload(id));
451-465: Create a per-request registry instead of mutating the global instance.As previously noted, mutating the shared global
registrycan cause race conditions when multiple signing requests are processed concurrently. Each request should use its own isolated TypeRegistry instance.Apply the previously suggested refactor:
- private handleRegistry (payload: SignerPayloadJSON): void { - // Get the metadata for the genesisHash - const currentMetadata = this.metadataGet(payload.genesisHash); - - // set the registry before calling the sign function - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); + const currentMetadata = this.metadataGet(payload.genesisHash); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + + return reg; + }
521-540: Guard against missing pairs and use a per-request registry.As previously noted:
keyring.getPair(address)can throw for unknown accounts - should return null insteadhandleRegistrymutates the global registry, causing potential race conditionsApply the previously suggested fix (adapted for the refactored
createSigningRegistry):private getSignature ({ payload }: RequestSigninSignature): HexString | null { const { address } = payload; if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } - const pair = keyring.getPair(address); this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); return signature; }
🧹 Nitpick comments (5)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx (1)
78-78: LGTM! Label change improves clarity.The label change from "Enable Auto-Lock" to "Adjust Auto-Lock" better reflects that this control manages both the enabled state and timer configuration, not just enabling the feature.
Optional formatting nitpick: Remove the space after the
=sign for consistency with standard JSX formatting conventions (label={t('Adjust Auto-Lock')}).packages/extension-polkagate/src/hooks/index.ts (1)
47-47: Clarify theuseIsFlyinghook nameRename
useIsFlyingto something more descriptive—e.g.useIsInitializing,useIsBooting, oruseIsStartingUp—to better convey its purpose.packages/extension-polkagate/src/hooks/useAutoLock.ts (2)
32-32: Add runtime validation for storage data.The type assertion
as AutoLock|undefinedbypasses compile-time type checking. IfgetStoragereturns malformed or unexpected data (e.g., from corrupted storage or version migration), this could lead to runtime issues downstream when the invalid object is used.Consider adding a type guard or validation function:
+function isValidAutoLock(value: unknown): value is AutoLock { + if (!value || typeof value !== 'object') return false; + const obj = value as Record<string, unknown>; + return ( + typeof obj.enabled === 'boolean' && + typeof obj.delay === 'object' && + obj.delay !== null && + typeof (obj.delay as Record<string, unknown>).value === 'number' && + ['min', 'hour', 'day'].includes((obj.delay as Record<string, unknown>).type as string) + ); +} + useEffect(() => { getStorage('autoLock').then((res) => { - setAutoLock(res as AutoLock|undefined ?? DEFAULT_AUTO_LOCK); + setAutoLock(isValidAutoLock(res) ? res : DEFAULT_AUTO_LOCK); }).catch(console.error);
35-41: Apply the same validation to storage change events.The type assertion on line 37 has the same type-safety concern as line 32. The Chrome storage API doesn't enforce type constraints, so
newValueshould be validated before use.If you implement the validation guard suggested for line 32, apply it here as well:
chrome.storage.onChanged.addListener(function (changes, areaName) { if (areaName === 'local' && 'autoLock' in changes) { - const newValue = changes['autoLock'].newValue as AutoLock; - - setAutoLock(newValue); + const newValue = changes['autoLock'].newValue; + if (isValidAutoLock(newValue)) { + setAutoLock(newValue); + } } });packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (1)
41-41: Consider using theme colors instead of hardcoded values.The color logic is now cleaner, but hardcoded hex values
'#AA83DC'and'#745D8B'could be replaced with theme constants for better maintainability and consistency.If your theme defines these purple shades, reference them instead:
- <Unlock color={isDark ? '#AA83DC' : '#745D8B'} size={18} variant='Bulk' /> + <Unlock color={theme.palette.primary.light} size={18} variant='Bulk' />Or add the colors to your theme if they're not already defined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/extension-base/src/background/handlers/Extension.ts(7 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx(1 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useAssetsBalances.ts(2 hunks)packages/extension-polkagate/src/hooks/useAutoLock.ts(1 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsLoginEnabled.ts(0 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx(1 hunks)packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx(3 hunks)packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx(2 hunks)packages/extension/public/locales/en/translation.json(1 hunks)packages/extension/public/locales/es/translation.json(0 hunks)packages/extension/public/locales/fr/translation.json(0 hunks)packages/extension/public/locales/hi/translation.json(0 hunks)packages/extension/public/locales/ru/translation.json(0 hunks)packages/extension/public/locales/zh/translation.json(0 hunks)
💤 Files with no reviewable changes (6)
- packages/extension/public/locales/ru/translation.json
- packages/extension/public/locales/es/translation.json
- packages/extension/public/locales/fr/translation.json
- packages/extension/public/locales/hi/translation.json
- packages/extension/public/locales/zh/translation.json
- packages/extension-polkagate/src/hooks/useIsLoginEnabled.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/util/types.ts (2)
DropdownOption(877-880)UserAddedChains(722-722)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (3)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-base/src/background/handlers/Extension.ts (1)
accountsValidate(225-233)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)
packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useAssetsBalances.ts (1)
useAssetsBalances(38-171)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
packages/extension-polkagate/src/hooks/index.ts (2)
51-51: LGTM!The export is correctly formatted, alphabetically ordered, and the hook name clearly conveys its purpose.
55-55: LGTM!The export is correctly formatted, alphabetically ordered, and the hook name clearly conveys its purpose for providing login flow information.
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
38-38: LGTM: Parameter addition is sound.The optional
isExtensionLockedparameter maintains backward compatibility and follows good naming conventions.
47-47: AddshouldFetchAssetsto the useEffect dependency array.The computed value
shouldFetchAssetsdepends onisExtensionLocked, but it's not listed in the useEffect dependency array at line 168. When the extension is unlocked (i.e.,isExtensionLockedtransitions fromtruetofalse), the useEffect won't re-trigger to fetch assets because it only sees the other dependencies.Apply this diff to add the missing dependency:
- }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions]); + }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions, isExtensionLocked, pathname]);Alternatively, if you prefer to keep the dependency list minimal, add only
shouldFetchAssets:- }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions]); + }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions, shouldFetchAssets]);However, the first approach (adding
isExtensionLockedandpathname) is clearer because it makes the actual dependencies explicit rather than relying on the derived value.Likely an incorrect or invalid review comment.
packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx (1)
10-10: LGTM: Lock state integration is correct.The changes properly:
- Import the lock context hook
- Retrieve the lock state
- Pass it to the assets balance hook
The integration follows React best practices and correctly threads the lock state through the component hierarchy.
Also applies to: 22-23
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (2)
44-44: LGTM!Simplifying to always use
'text.primary'improves consistency and ensures proper theming.
13-13: Safe removal ofuseIsLoginEnabled
Verified no imports or usages remain in.ts/.tsxfiles; removal is safe.packages/extension-base/src/background/handlers/Extension.ts (3)
8-8: LGTM!The new type imports align with the new method signatures and are properly defined in the background types.
Also applies to: 11-11
439-449: LGTM!The lock expiration logic correctly handles edge cases: returns false when all accounts are external, and properly checks cached unlock times against the current time.
690-705: LGTM!The new message handlers correctly route to the corresponding methods with appropriate type casting, following the established pattern in this file.
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (7)
packages/extension-base/src/background/handlers/Extension.ts (6)
136-145: Locking fails when cached entry belongs to a forgotten account
keyring.getPair(address)throws once an unlocked account gets forgotten, but we keep the stale cache entry. The thrown error aborts the loop, so the extension never finishes locking remaining accounts (and the request rejects). Please guard missing pairs and purge the stale cache entry before continuing.- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + + if (!pair.isLocked) { + pair.lock(); + } + + this.#cachedUnlocks[address] = 0; + } catch (error) { + delete this.#cachedUnlocks[address]; + } + });
163-163: chrome.tabs.reload does not return a Promise;.catchwill throw.Use the existing withErrorLog helper or a callback to avoid runtime errors.
- chrome.tabs.reload(id).catch(console.error); + withErrorLog(() => chrome.tabs.reload(id));
407-443: Handle individual account unlock failures gracefully.If
pair.decodePkcs8(password)throws for any account (e.g., wrong password, corrupted keystore), the entire operation fails and no accounts are unlocked. Consider wrapping the unlock logic in a try-catch to handle failures per account and return partial success information.- const res = localAccounts.map(({ address }) => { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - - this.#cachedUnlocks[address] = Date.now() + cacheTime; - - return true; - }); + const res = localAccounts.map(({ address }) => { + try { + const pair = keyring.getPair(address); + + if (!pair) { + return false; + } + + if (pair.isLocked) { + pair.decodePkcs8(password); + } + + this.#cachedUnlocks[address] = Date.now() + cacheTime; + + return true; + } catch (error) { + console.error(`Failed to unlock account ${address}:`, error); + return false; + } + });
457-471: Avoid global TypeRegistry mutation; create a per-request registry for signing.Global registry is shared across concurrent requests and may be reconfigured mid‑sign, causing non‑deterministic behavior. Build and use a local registry per payload.
- private handleRegistry (payload: SignerPayloadJSON): void { - // Get the metadata for the genesisHash - const currentMetadata = this.metadataGet(payload.genesisHash); - - // set the registry before calling the sign function - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); + const currentMetadata = this.metadataGet(payload.genesisHash); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + + return reg; + }
506-506: Use the per-request registry from the refactored method.Once
handleRegistryis refactored tocreateSigningRegistry(returning a local registry), update this call site to use the returned registry for signing instead of the global instance.- this.handleRegistry(payload); + const reg = this.createSigningRegistry(payload);Then pass
regtorequest.sign(reg, pair)on Line 509.
527-546: Harden getSignature: handle unknown accounts and use a local registry.
keyring.getPair(address)can throw; return null instead of throwing.- Use a per‑request registry to avoid races.
- Remove global registry mutation.
- private getSignature ({ payload }: RequestSigninSignature): HexString | null { - const { address } = payload; - - if (!payload?.address || !payload.genesisHash) { - throw new Error('Invalid payload: missing required fields.'); - } - - const pair = keyring.getPair(address); - - this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired - - if (pair.isLocked) { - return null; - } - - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); - - return signature; - } + private getSignature ({ payload }: RequestSigninSignature): HexString | null { + if (!payload?.address || !payload.genesisHash) { + throw new Error('Invalid payload: missing required fields.'); + } + + const { address } = payload; + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } + + this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired + + if (pair.isLocked) { + return null; + } + + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); + + return signature; + }packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (1)
105-109: Resolved: unlocking state now resets via finallysetUnlocking(false) and resetting ref in finally addresses the spinner-stuck issue from the prior review.
🧹 Nitpick comments (4)
packages/extension-base/src/background/handlers/Extension.ts (1)
408-408: Remove debug console.log statements.Production code should not contain debug logging statements.
- console.log('get message to unlock accounts ...');- console.log('localAccounts:', localAccounts);Also applies to: 419-419
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (3)
96-99: Avoid type-cast; use void operator for ignored navigate resultCleaner and avoids unnecessary casting.
- : navigate(path) as void; + : void navigate(path);
85-86: Prefer void operator with error handling over type-cast for setStorageSuppressing the Promise via cast hides errors; use void and catch.
- setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error);
156-160: Disable Unlock until prerequisites are ready (better UX)Prevent no-op Enter/clicks before autoLockPeriod and isPasswordMigrated resolve.
- disabled={!hashedPassword} + disabled={ + !hashedPassword || + autoLockPeriod === undefined || + isPasswordMigrated === undefined + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension-base/src/background/handlers/Extension.ts(7 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (10)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(29-47)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(18-23)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(96-98)windowOpen(259-261)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(71-84)updateStorage(19-36)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
LOGIN_STATUS(4-11)packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (1)
openOrFocusTab(35-60)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
[error] 53-53: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 123-124: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (1)
7-9: react-router-dom is v7.8.1 (v6+), useNavigate is supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
♻️ Duplicate comments (11)
packages/extension-polkagate/src/components/Loading.tsx (1)
102-102: Allow-list should ignore search/hash suffixes.This is the same issue flagged in the previous review:
ALLOWED_URL_ON_RESET_PASSWORD.includes(extensionUrl)only matches exact strings. When routes carry query params (e.g.,/forgot-password?address=...), the check fails and the login overlay incorrectly appears, blocking the reset flow.Apply the previously suggested fix:
- const isResettingWallet = isForgotten?.status || ALLOWED_URL_ON_RESET_PASSWORD.includes(window.location.hash.replace('#', '')); + const extensionUrl = window.location.hash.replace('#', ''); + const currentPath = extensionUrl.split('?')[0]; + const isResettingWallet = isForgotten?.status || + ALLOWED_URL_ON_RESET_PASSWORD.some((allowed) => currentPath.startsWith(allowed));packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
6-6: Guard undefined/empty accounts, reset state on missing pass, and add cleanupAvoid calling with undefined/empty accounts, reset state when
passis missing, and prevent stale updates with a cancellation guard. This aligns with the earlier review note.-import { useContext, useEffect, useState } from 'react'; +import { useContext, useEffect, useMemo, useState } from 'react'; @@ - const localAccounts = accounts.filter(({ isExternal }) => !isExternal); + const localAccounts = useMemo( + () => (accounts || []).filter(({ isExternal }) => !isExternal), + [accounts] + ); @@ useEffect(() => { - if (!pass) { - return; - } - - setAccountsNeedMigration(undefined); - - getAccountsNeedsMigration(localAccounts, pass).then((res) => { - setAccountsNeedMigration(res); - }).catch(console.error); + if (!pass || !accounts?.length) { + setAccountsNeedMigration(undefined); + return; + } + + let isCurrent = true; + setAccountsNeedMigration(undefined); + + void getAccountsNeedsMigration(localAccounts, pass) + .then((res) => { if (isCurrent) setAccountsNeedMigration(res); }) + .catch((e) => { console.error(e); if (isCurrent) setAccountsNeedMigration([]); }); + + return () => { isCurrent = false; }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [accounts?.length, pass]);Also applies to: 26-26, 30-41
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (3)
83-96: Clear secrets from state and handle storage Promises explicitly
- Keep plaintext and hash lifetime minimal; clear both after success/migration.
- Don’t discard Promises with a cast; catch errors. Use
voidto drop result.if (success) { setExtensionLock(false); - hasLocalAccounts && setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + if (hasLocalAccounts) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error); + } + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); + setHashedPassword(undefined); } else { setExtensionLock(true); setIsPasswordError(true); } - setPlainPassword(undefined); + setPlainPassword(undefined); } else if (accountsNeedMigration?.length && isOldPasswordCorrect) { // needs migration await updateStorage(STORAGE_KEY.LOGIN_INFO, { lastLoginTime: Date.now(), status: LOGIN_STATUS.SET }); // DEPRECATED, will be removed in future releases - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); setHashedPassword(undefined); + setPlainPassword(undefined); setExtensionLock(false); const path = '/migratePasswords'; isExtension ? windowOpen(path).catch(console.error) - : navigate(path) as void; + : void navigate(path);Also applies to: 100-103
51-61: Fix void callback: don’t return setter and clear stale plaintext on empty inputReturning
setHashedPassword(...)from a void handler breaks lint and leavesplainPasswordstale.Apply:
const onPassChange = useCallback((pass: string | null): void => { if (!pass) { - return setHashedPassword(undefined); + setHashedPassword(undefined); + setPlainPassword(undefined); + return; } setPlainPassword(pass); setIsPasswordError(false); const hashedPassword = blake2AsHex(pass, 256); // Hash the string with a 256-bit output setHashedPassword(hashedPassword); }, []);
125-133: Fix void callback: don’t return setter in onForgotPasswordReturning
setStep(...)from a void handler breaks lint.const onForgotPassword = useCallback((): void => { if (isExtension) { - return setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION); + setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION); + return; } setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION_FS); openOrFocusTab('/forgot-password', true); }, [isExtension, setStep]);packages/extension-base/src/background/handlers/Extension.ts (6)
146-155: Lock loop can throw on forgotten/missing pairs; purge stale cache and continue
keyring.getPair(address)may throw if an account was forgotten; this aborts locking the rest.- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + if (!pair.isLocked) { + pair.lock(); + } + this.#cachedUnlocks[address] = 0; + } catch { + delete this.#cachedUnlocks[address]; + } + });
173-175: chrome.tabs.reload is callback-style;.catchwill throwUse helper or callback to log errors; remove
.catch.- chrome.tabs.reload(id).catch(console.error); + withErrorLog(() => chrome.tabs.reload(id));
428-456: Unlock-all should handle per-account failures and continueA single
decodePkcs8error aborts the whole batch; wrap each attempt to allow partial progress and return aggregate result.- try { - const accountsLocal = this.localAccounts(); - const res = accountsLocal.map(({ address }) => { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - - this.#cachedUnlocks[address] = Date.now() + cacheTime; - - return true; - }); - - return res.every((success) => success); - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + const accountsLocal = this.localAccounts(); + const res = accountsLocal.map(({ address }) => { + try { + const pair = keyring.getPair(address); + if (pair.isLocked) { + pair.decodePkcs8(password); + } + this.#cachedUnlocks[address] = Date.now() + cacheTime; + return true; + } catch (error) { + console.error(`Failed to unlock ${address}:`, error); + return false; + } + }); + return res.every(Boolean);
459-467: Expiry check ignores local accounts not present in cacheIterating cache entries only can report expired while some locals remain unlocked/uncached. Check all local accounts.
- return Object.entries(this.#cachedUnlocks).every(([, time]) => time < Date.now()); + return accountsLocal.every(({ address }) => { + const until = this.#cachedUnlocks[address]; + return !until || until < Date.now(); + });
469-483: Avoid mutating global TypeRegistry; use a per-request registryGlobal
registryis shared across requests and can be reconfigured mid-sign. Build a local registry and use it for signing.Replace handleRegistry with a factory and use it:
- private handleRegistry (payload: SignerPayloadJSON): void { + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { const currentMetadata = this.metadataGet(payload.genesisHash); - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + const reg = new TypeRegistry(); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + return reg; + }And in password-approve path:
- if (isJsonPayload(payload)) { - this.handleRegistry(payload); - } - - const result = request.sign(registry, pair); + let reg = registry; + if (isJsonPayload(payload)) { + reg = this.createSigningRegistry(payload); + } + const result = request.sign(reg, pair);Also applies to: 517-522
539-558: Harden getSignature: validate first, guard getPair, and use local registry
- Don’t destructure before null checks.
keyring.getPaircan throw; return null.- Use per-request registry, not the shared global.
- private getSignature ({ payload }: RequestSigninSignature): HexString | null { - const { address } = payload; - - if (!payload?.address || !payload.genesisHash) { - throw new Error('Invalid payload: missing required fields.'); - } - - const pair = keyring.getPair(address); + private getSignature ({ payload }: RequestSigninSignature): HexString | null { + if (!payload?.address || !payload.genesisHash) { + throw new Error('Invalid payload: missing required fields.'); + } + const { address } = payload; + let pair: KeyringPair; + try { + pair = keyring.getPair(address); + } catch { + return null; + } this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); return signature; }
🧹 Nitpick comments (14)
packages/extension-polkagate/src/components/Loading.tsx (1)
20-85: Relocate utility functions to a dedicated file.These storage utility functions (
updateStorage,getStorage,watchStorage,setStorage) are exported but defined within a component file. They should be moved to a separate utility module (e.g.,util/storage.ts) for better maintainability and reusability.packages/extension-polkagate/src/fullscreen/home/index.tsx (1)
24-30: Ensure navigation waits for real account data
AccountContext defaultsaccountsto[]—it’s neverundefined, soaccounts?.length === 0always fires on mount. Introduce a loading or null‐check (e.g. guard on the rawaccountsstate) before redirecting to avoid premature onboarding navigation.packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx (2)
98-101: Handle storage-set errors consistentlyAvoid casting away the promise; add error handling like the previous storage write.
- setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; + setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error);
208-225: Password entry UX looks good; minor nitConditional MatchPasswordField vs PasswordInput is appropriate. Optionally consider passing a controlled value to PasswordInput for consistency with other controlled fields.
packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (4)
38-39: Avoid name collision with isPasswordCorrectShadowing the exported function name is confusing. Alias the hook result and update usages.
- const { isPasswordCorrect } = useIsPasswordCorrect(oldPass, readyToGo); + const { isPasswordCorrect: isOldPassValid } = useIsPasswordCorrect(oldPass, readyToGo); @@ - if (!isPasswordCorrect) { + if (!isOldPassValid) { @@ - }, [isPasswordCorrect, newPass, oldPass, readyToGo, t]); + }, [isOldPassValid, newPass, oldPass, readyToGo, t]);Also applies to: 50-73
5-10: Remove legacy storage-based password check helperWith useIsPasswordCorrect and accountsChangePasswordAll in place, the exported isPasswordCorrect helper (LOGIN_INFO + blake2) is obsolete and risks drift. Drop it and clean up imports.
-import React, { useCallback, useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; @@ -import useIsPasswordCorrect from '@polkadot/extension-polkagate/src/hooks/useIsPasswordCorrect'; -import { accountsChangePasswordAll } from '@polkadot/extension-polkagate/src/messaging'; -import { getStorage, setStorage } from '@polkadot/extension-polkagate/src/util'; +import useIsPasswordCorrect from '@polkadot/extension-polkagate/src/hooks/useIsPasswordCorrect'; +import { accountsChangePasswordAll } from '@polkadot/extension-polkagate/src/messaging'; +import { setStorage } from '@polkadot/extension-polkagate/src/util'; @@ -import { blake2AsHex } from '@polkadot/util-crypto'; +// (removed blake2 import) @@ -import { type LoginInfo } from '../../passwordManagement/types'; +// (removed unused LoginInfo type) @@ -export const isPasswordCorrect = async (password: string, isHashed?: boolean) => { - const hashedPassword = isHashed ? password : blake2AsHex(password, 256); - const info = await getStorage(STORAGE_KEY.LOGIN_INFO) as LoginInfo; - - return info?.hashedPassword === hashedPassword; -}; +// removed legacy isPasswordCorrect()Also applies to: 14-18, 20-25
86-93: Optional: make current password field controlledKeeps state in sync and simplifies debugging.
- <PasswordInput + <PasswordInput + value={oldPass} focused hasError={passwordError} onPassChange={onCurrentPasswordChange} style={{ marginBottom: '18px' }} title={t('Current Password')} />
93-99: Remove ts-ignore by fixing MatchPasswordField prop typesDon’t suppress types. Update MatchPasswordField’s onSetPassword signature to accept a no-arg callback (or the expected type) and drop the ts-ignore.
packages/extension-polkagate/src/components/PasswordInput.tsx (1)
80-83: Simplify onChangeValue is always a string; remove the null coalescing for clarity.
- const onChange = useCallback(({ target: { value } }: React.ChangeEvent<HTMLInputElement>) => { - onPassChange(value ?? null); - }, [onPassChange]); + const onChange = useCallback(({ target: { value } }: React.ChangeEvent<HTMLInputElement>) => { + onPassChange(value); + }, [onPassChange]);packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
22-35: Prevent stale state updates; add cleanup and narrow depsAdd a cancellation guard to avoid setting state after unmount/race, use
firstLocal.addressdirectly, and narrow deps.useEffect(() => { setCorrect(undefined); if (!password || ready === false || !firstLocal) { return; } - accountsValidate(localAccounts[0].address, password).then((status) => { - setCorrect(status); - }).catch((error) => { - console.error(error); - setCorrect(false); - }); - }, [firstLocal, localAccounts, password, ready]); + let isCurrent = true; + void accountsValidate(firstLocal.address, password) + .then((status) => { if (isCurrent) setCorrect(status); }) + .catch((error) => { console.error(error); if (isCurrent) setCorrect(false); }); + + return () => { isCurrent = false; }; + }, [firstLocal?.address, password, ready]);packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (1)
26-36: Robust timestamp parsing and placeholderHandle numeric/string timestamps safely and fix the placeholder format.
- getStorage(STORAGE_KEY.LAST_PASS_CHANGE).then((timestamp) => { - if (timestamp) { - const date = new Date(timestamp as unknown as number); + getStorage(STORAGE_KEY.LAST_PASS_CHANGE).then((timestamp) => { + const ms = Number(timestamp); + if (Number.isFinite(ms) && ms > 0) { + const date = new Date(ms); @@ - <Typography color='text.secondary' variant='B-5'> - {lastEditDate ?? '__,__,__'} + <Typography color='text.secondary' variant='B-5'> + {lastEditDate ?? '__.__.__'}Also applies to: 73-73
packages/extension-polkagate/src/messaging.ts (1)
84-87: Avoid duplicate wrappers for the same messageBoth
accountsValidateandvalidateAccountcallpri(accounts.validate). This duplicates API surface and invites confusion.Options:
- Prefer
accountsValidateand removevalidateAccount, or- Re-export for backward compat:
-export async function validateAccount (address: string, password: string): Promise<boolean> { - return sendMessage('pri(accounts.validate)', { address, password }); -} +// Deprecated: use accountsValidate +export const validateAccount = accountsValidate;Also applies to: 125-127
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (2)
146-149: Remove ts-ignore by aligning callback typeAvoid
// @ts-ignoreonMatchPasswordField. If it expects a parameter, wrap your handler to accept and ignore it, or update prop typing.Example:
- // @ts-ignore - onSetPassword={onContinue} + onSetPassword={(): void => onContinue()}Or adjust
MatchPasswordFieldprop type to accept() => voidas well.
56-58: Prefervoid navigate(...)and clear secrets on completionMinor cleanup for intent clarity; also clear in-memory passwords when done.
- navigate('/') as void; + void navigate('/');setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) .then(() => { setExtensionLock(true); - navigate('/') as void; - lockExtension().catch(console.error); + void navigate('/'); + void lockExtension().catch(console.error); + // minimize secret lifetime + setCurrentPass(''); + setMasterPass(undefined); }).catch(console.error);Also applies to: 101-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
packages/extension-base/src/background/handlers/Extension.ts(9 hunks)packages/extension-base/src/background/types.ts(5 hunks)packages/extension-polkagate/src/components/Loading.tsx(2 hunks)packages/extension-polkagate/src/components/PasswordInput.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/home/index.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx(2 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsForgotten.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts(1 hunks)packages/extension-polkagate/src/hooks/useLocalAccounts.ts(1 hunks)packages/extension-polkagate/src/messaging.ts(3 hunks)packages/extension-polkagate/src/popup/home/index.tsx(4 hunks)packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx(3 hunks)packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx(4 hunks)packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts(2 hunks)packages/extension-polkagate/src/popup/passwordManagement/FirstTimeSetPassword.tsx(0 hunks)packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx(2 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/Reset.tsx(2 hunks)packages/extension-polkagate/src/popup/passwordManagement/constants.ts(0 hunks)packages/extension-polkagate/src/popup/passwordManagement/types.ts(1 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx(3 hunks)packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx(2 hunks)packages/extension-polkagate/src/util/constants.ts(2 hunks)packages/extension-ui/src/Popup/contexts/AccountProvider.tsx(3 hunks)packages/extension/public/locales/en/translation.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/extension-polkagate/src/popup/passwordManagement/constants.ts
- packages/extension-polkagate/src/popup/passwordManagement/FirstTimeSetPassword.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
🧰 Additional context used
🧬 Code graph analysis (22)
packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx (4)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/components/Loading.tsx (1)
updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/hooks/useLocalAccounts.ts (2)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/components/contexts.tsx (1)
AccountContext(34-34)
packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx (3)
packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (3)
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)packages/extension-polkagate/src/components/Loading.tsx (2)
watchStorage(51-70)getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(236-239)RequestUnlockAllAccounts(408-411)RequestSigninSignature(319-321)RequestUpdateMeta(356-359)
packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (5)
packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-41)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)packages/extension-base/src/background/handlers/Extension.ts (1)
accountsChangePasswordAll(92-100)packages/extension-polkagate/src/messaging.ts (1)
accountsChangePasswordAll(92-94)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (3)
packages/extension-polkagate/src/util/handleExtensionPopup.ts (1)
useExtensionPopups(11-18)packages/extension-polkagate/src/components/Loading.tsx (1)
getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/popup/passwordManagement/Reset.tsx (3)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)packages/extension-polkagate/src/messaging.ts (1)
lockExtension(80-82)
packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (2)
packages/extension-base/src/background/handlers/Extension.ts (1)
localAccounts(422-426)packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (3)
packages/extension-polkagate/src/components/Loading.tsx (2)
getStorage(39-49)updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)
packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (2)
packages/extension-base/src/background/handlers/Extension.ts (2)
localAccounts(422-426)accountsValidate(240-248)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)
packages/extension-base/src/background/types.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-ui/src/Popup/contexts/AccountProvider.tsx (3)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
useIsForgotten(11-20)packages/extension-polkagate/src/components/Loading.tsx (1)
updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx (1)
packages/extension-polkagate/src/messaging.ts (1)
deriveAccount(259-261)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (8)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-47)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(102-120)lockExtension(145-180)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (4)
packages/extension-base/src/background/handlers/Extension.ts (2)
localAccounts(422-426)accountsValidate(240-248)packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)packages/extension-polkagate/src/components/contexts.tsx (1)
AccountContext(34-34)
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (2)
packages/extension-base/src/background/handlers/Extension.ts (1)
lockExtension(145-180)packages/extension-polkagate/src/messaging.ts (1)
lockExtension(80-82)
packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx (4)
packages/extension-base/src/background/types.ts (1)
ResponseJsonGetAccountInfo(454-459)packages/extension-polkagate/src/messaging.ts (1)
unlockAllAccounts(100-102)packages/extension-polkagate/src/util/constants.ts (2)
AUTO_LOCK_PERIOD_DEFAULT(226-226)STORAGE_KEY(302-322)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)
packages/extension-polkagate/src/popup/home/index.tsx (1)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
useIsForgotten(11-20)
packages/extension-polkagate/src/components/Loading.tsx (6)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
useIsFlying(10-30)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
useIsForgotten(11-20)packages/extension-polkagate/src/util/constants.ts (1)
ALLOWED_URL_ON_RESET_PASSWORD(230-230)packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx (1)
ForgotPassword(72-95)
packages/extension-polkagate/src/messaging.ts (2)
packages/extension-base/src/background/handlers/Extension.ts (4)
accountsValidate(240-248)accountsChangePassword(102-120)accountsChangePasswordAll(92-100)getSignature(539-558)packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (8)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-47)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(20-25)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(100-102)windowOpen(263-265)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(72-85)updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
[error] 53-53: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
[error] 53-53: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 127-128: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (24)
packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx (1)
127-127: LGTM! Simplified password label aligns with master password feature.The change from a dynamic translation (likely including the parent account name) to a static "Password" label is appropriate, especially given the master password implementation mentioned in the PR objectives. The
SelectAccountcomponent above (lines 114-121) already provides visual context about which account's password is being requested, so the simpler label maintains clarity while supporting the unified password workflow.packages/extension-polkagate/src/popup/home/index.tsx (4)
9-9: LGTM: Clean import addition.The import of
useIsForgottenhook is correctly added to support the new forgotten-password flow.
24-24: LGTM: Explicit void cast clarifies intent.The explicit
as voidcast clearly communicates that the return value ofuseBackgroundis intentionally unused here. This is a clean way to indicate side-effect-only hook usage.
33-33: LGTM: Hook usage follows React patterns.The
useIsForgottenhook is correctly invoked to retrieve the forgotten-password state from storage.
70-72: LGTM: Conditional logic correctly handles forgotten state.The refactored rendering logic cleanly replaces the previous LOGIN_INFO flow with the new
isForgotten?.statuscheck. The optional chaining correctly handles the initialundefinedstate when storage hasn't loaded yet, defaulting to showing the Welcome component until the storage read completes.packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx (3)
87-87: LGTM!The simplified heading text appropriately reflects the master password flow where users no longer need to set a separate password for the derived account.
113-113: LGTM!The disabled state logic is correctly simplified to only check for
childName, consistent with removing the password requirement from the UI.
47-61: Master password flow verified – no changes needed.The backend’s
derivationCreateusesparentPasswordto derive the child key and thepasswordparameter to encrypt it. PassingparentPasswordfor both meets the master-password design without validation issues.packages/extension-polkagate/src/components/Loading.tsx (2)
96-100: Verify step state management.The effect only sets
steptoSTEPS.SHOW_LOGINwhen the extension becomes locked, but doesn't clear or updatestepwhenisExtensionLockedbecomesfalse. Ensure this doesn't cause stale state issues if the extension is unlocked while the component is mounted.
117-119: Original review comment is incorrect:Login’s onForgotPassword handler calls setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION), so ForgotPassword does render as expected.Likely an incorrect or invalid review comment.
packages/extension-polkagate/src/fullscreen/home/index.tsx (1)
22-22: LGTM! Clean context integration.The transition to
AccountContextfor account data aligns well with the master password migration architecture described in the PR.packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx (1)
52-76: No layout or styling impact from added wrapper div – there are no CSS selectors or spacing rules targeting the Container as a direct child, so the extra<div>does not affect the flex column layout.packages/extension-polkagate/src/components/PasswordInput.tsx (1)
97-101: Layout wrapper changeSwitching to Grid wrapper can affect spacing/margins. Please eyeball a few screens using PasswordInput to ensure no regressions.
packages/extension-polkagate/src/hooks/index.ts (1)
47-55: New exports approved; useIsLoginEnabled removal verified
Search across.ts/.tsxfiles shows no remaining imports or usages ofuseIsLoginEnabled.packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
23-26: LGTM!The new
ForgottenInfointerface is well-defined and properly supports the forgotten-password workflow. The tri-statestatusfield (boolean | undefined) appropriately handles initialization scenarios.packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (1)
198-215: LGTM!The conditional rendering correctly handles the master-password migration flow:
- New users (no local accounts) set up their initial password via
MatchPasswordField- Existing users (with local accounts) enter their existing password via
PasswordInputThe optional chaining on Line 198 properly handles the undefined case.
packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx (1)
33-36: LGTM!The storage update correctly uses the new
IS_FORGOTTENkey with the appropriate data structure. The flow properly clears the extension lock before navigation.packages/extension-polkagate/src/popup/passwordManagement/Reset.tsx (1)
112-120: LGTM!The refactoring from
ActionContexttouseNavigatefollows modern React Router patterns. The storage update correctly resets theIS_FORGOTTENstate before re-locking the extension.packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
13-17: Verify that the shallow dependency is safe.The
useMemodepends onaccounts?.lengthrather than the fullaccountsarray. This optimization assumes that theisExternalproperty of existing accounts doesn't change during the component lifecycle.While this is typically safe (accounts don't usually flip between external/internal), consider whether there are any edge cases where an account's
isExternalstatus might change without modifying the array length.If there are no such scenarios, this optimization is appropriate. You may want to add a comment explaining the rationale:
return useMemo( () => accounts.filter(({ isExternal }) => !isExternal), - // eslint-disable-next-line react-hooks/exhaustive-deps + // Only re-filter when account count changes; isExternal property is immutable after account creation + // eslint-disable-next-line react-hooks/exhaustive-deps [accounts?.length] );packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
11-19: LGTM!The hook correctly implements real-time storage synchronization:
- Subscribes to storage changes via
watchStorage- Retrieves initial value on mount
- Properly handles the tri-state return type
The nullish coalescing operator on Line 16 appropriately provides a fallback to
nullwhen storage is empty.packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
20-37: LGTM!The refactoring improves error handling with
Promise.all:
- Executes account forgetting operations in parallel for better performance
- Validates that all operations succeeded before returning
- Properly catches and logs errors
The storage update correctly resets the
IS_FORGOTTENstatus before processing the account deletions.packages/extension-polkagate/src/util/constants.ts (1)
310-313: Storage keys: validate writes/reads are updatedDouble-check read/write sites use
IS_FORGOTTEN,IS_PASSWORD_MIGRATED, andLAST_PASS_CHANGE.You can extend the script above to search for usages and mismatches.
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (1)
109-115: Good: unlocking state reset in finally
setUnlocking(false)andisUnlockingRef.current = falsein finally reliably clear busy state after all paths. LGTM.packages/extension-base/src/background/types.ts (1)
105-112: Types and mappings look consistent with new handlersNew request interfaces and pri(...) mappings align with handlers and messaging. No issues spotted.
Also applies to: 121-123, 236-240, 408-411
packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (1)
26-26: Consider safer type handling for timestamp.The double cast
as unknown as numberbypasses type safety. If the stored value is a string representation of a timestamp, theDateconstructor will handle it, but this defeats TypeScript's type checking.Consider one of these approaches:
Option 1: Type guard with explicit conversion
- const date = new Date(timestamp as unknown as number); + const timestampNum = typeof timestamp === 'number' ? timestamp : parseInt(String(timestamp), 10); + const date = new Date(timestampNum);Option 2: Let Date constructor handle it (it accepts string | number)
- const date = new Date(timestamp as unknown as number); + const date = new Date(timestamp as string | number);Option 3: Update
getStoragereturn type to includenumberin its signature (if timestamps are always stored as numbers).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (4)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/util/handleExtensionPopup.ts (1)
useExtensionPopups(11-18)packages/extension-polkagate/src/components/Loading.tsx (1)
getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (1)
34-34: Good addition of error handling.The
.catch(console.error)prevents unhandled promise rejection and logs errors. The fallback display'__,__,__'(line 71) provides adequate user feedback when the timestamp is unavailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-base/src/background/handlers/Extension.ts (1)
521-526: Use the per‑request registry during signingSwitch from global registry to the local one returned above.
- if (isJsonPayload(payload)) { - this.handleRegistry(payload); - } - - const result = request.sign(registry, pair); + const reg = isJsonPayload(payload) + ? this.createSigningRegistry(payload) + : new TypeRegistry(); + + const result = request.sign(reg, pair);
♻️ Duplicate comments (16)
packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (1)
41-53: Critical: Validation persists keys to storage.The
keyring.addUrimethod persists keys to the keyring as a side effect. Validation should be pure and not mutate storage. Additionally, the error message is not translated.Use the background
validateSeedmethod instead:+import { validateSeed as validateSeedBg } from '../../../messaging'; + const validateSeed = useCallback(async (seed: string, type?: KeypairType): Promise<AccountInfo> => { if (!(seed.startsWith('0x') && seed.length === 66)) { - throw new Error('The raw seed is invalid. It should be 66 characters long and start with 0x'); + throw new Error(t('The raw seed is invalid. It should be 66 characters long and start with 0x')); } - const { pair } = keyring.addUri(seed, undefined, {}, type || DEFAULT_TYPE); + const { address, suri } = await validateSeedBg(seed, type || DEFAULT_TYPE); return { - address: pair.address, + address, genesis: POLKADOT_GENESIS, - suri: seed + suri }; -}, []); +}, [t]);packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
29-41: Guard on falsy pass and add cancellation to avoid stale state updates
- When
passbecomes falsy, previous results linger; reset state and return early.- Cancel in‑flight validations when
passorlocalAccountschange to prevent late resolves overwriting newer state.useEffect(() => { - if (!pass) { - return; - } - - setAccountsNeedMigration(undefined); - - getAccountsNeedsMigration(localAccounts, pass) - .then(setAccountsNeedMigration) - .catch(console.error); + if (!pass) { + setAccountsNeedMigration(undefined); + return; + } + + setAccountsNeedMigration(undefined); + + let cancelled = false; + void getAccountsNeedsMigration(localAccounts, pass) + .then((res) => { + if (!cancelled) setAccountsNeedMigration(res); + }) + .catch((e) => { + console.error(e); + if (!cancelled) setAccountsNeedMigration([]); + }); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [localAccounts?.length, pass]); + return () => { cancelled = true; }; + }, [localAccounts?.length, pass]);packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (2)
128-133: LGTM: void callback no longer returns a valueThe early return now avoids returning the setter result.
86-96: Clear hashed password on success to minimize secret lifetime
plainPasswordis cleared, buthashedPasswordremains in memory after a successful unlock.if (success) { setExtensionLock(false); - hasLocalAccounts && setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + if (hasLocalAccounts) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error); + } + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); } else { setExtensionLock(true); setIsPasswordError(true); } - setPlainPassword(undefined); + setPlainPassword(undefined); + setHashedPassword(undefined);packages/extension-base/src/background/handlers/Extension.ts (7)
174-175: LGTM: usewithErrorLogforchrome.tabs.reloadAvoids
.catchon a non‑Promise API and logs lastError safely.
145-155: Locking can throw for forgotten accounts; purge stale cache and continue
keyring.getPair(address)may throw after an account is forgotten; the loop aborts and remaining accounts aren’t locked.- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + if (!pair.isLocked) { + pair.lock(); + } + this.#cachedUnlocks[address] = 0; + } catch { + delete this.#cachedUnlocks[address]; + } + });
433-461: Handle per‑account unlock failures without aborting the whole operationA single bad keystore/password currently flips the entire result to
false. Capture failures per account and keep going.- try { - const accountsLocal = this.localAccounts(); - const res = accountsLocal.map(({ address }) => { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (!pair.isLocked) { - pair.lock(); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - - this.#cachedUnlocks[address] = Date.now() + cacheTime; - - return true; - }); - - return res.every((success) => success); - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + const accountsLocal = this.localAccounts(); + const results = accountsLocal.map(({ address }) => { + try { + const pair = keyring.getPair(address); + if (!pair.isLocked) pair.lock(); + if (pair.isLocked) pair.decodePkcs8(password); + this.#cachedUnlocks[address] = Date.now() + cacheTime; + return true; + } catch (error) { + console.error(`accountsUnlockAll: failed to unlock ${address}`, error); + return false; + } + }); + + return results.every(Boolean);
463-471:areLocksExpiredignores accounts without cache entriesIterate local accounts and treat missing cache entries as expired.
- return Object.entries(this.#cachedUnlocks).every(([, time]) => time < Date.now()); + return accountsLocal.every(({ address }) => { + const time = this.#cachedUnlocks[address]; + return !time || time < Date.now(); + });
473-487: Avoid mutating the global TypeRegistry; create a per‑request registryGlobal registry mutation is a race across concurrent requests; use a local registry instead.
- private handleRegistry (payload: SignerPayloadJSON): void { - // Get the metadata for the genesisHash - const currentMetadata = this.metadataGet(payload.genesisHash); - - // set the registry before calling the sign function - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); + const currentMetadata = this.metadataGet(payload.genesisHash); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + return reg; + }
543-562: HardengetSignatureand sign with a local registryHandle unknown accounts and avoid global registry mutation.
private getSignature ({ payload }: RequestSigningSignature): HexString | null { if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } - const { address } = payload; - - const pair = keyring.getPair(address); - - this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired - - if (pair.isLocked) { - return null; - } - - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const { address } = payload; + let pair: KeyringPair; + try { + pair = keyring.getPair(address); + } catch { + return null; + } + + this.refreshAccountPasswordCache(pair); + if (pair.isLocked) { + return null; + } + + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); return signature; }
92-100: Gracefully handle per‑account failures inaccountsChangePasswordAllOne failure currently throws and aborts the whole request; return overall success while logging failures.
private accountsChangePasswordAll ({ newPass, oldPass }: RequestAccountChangePasswordAll): boolean { const accountsLocal = this.localAccounts(); - const res = accountsLocal.map(({ address }) => { - return this.accountsChangePassword({ address, newPass, oldPass }); - }); + const res = accountsLocal.map(({ address }) => { + try { + return this.accountsChangePassword({ address, newPass, oldPass }); + } catch (e) { + console.error(`accountsChangePasswordAll: failed for ${address}`, e); + return false; + } + }); return res.every(Boolean); }packages/extension-base/src/background/types.ts (2)
121-123: LGTM: consistent namingRequestSigningSignatureThe request/response map uses the corrected name.
361-364: LGTM:RequestUnlockAllAccountsplaced before Responses with clear commentInterface is in the Requests section and uses “milliseconds”.
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (3)
53-56: Don’t return a setter fromuseEffectReturning
setStep(...)is a no‑op and confuses the effect’s cleanup semantics.- if (accountsNeedMigration?.length) { - return setStep(STEP.MIGRATING); - } + if (accountsNeedMigration?.length) { + setStep(STEP.MIGRATING); + return; + }
185-192: NormalizePasswordInputnulls before updating string state
PasswordInputpassesstring | null; writingnullintostringstate is unsafe.- onPassChange={setCurrentPass} + onPassChange={(pass) => setCurrentPass(pass ?? '')}
37-39: AlignmasterPasstype with initializationAvoid
string | undefinedbut initialize with''. Pick one convention.- const [masterPass, setMasterPass] = useState<string | undefined>(''); + const [masterPass, setMasterPass] = useState<string>('');
🧹 Nitpick comments (6)
packages/extension-polkagate/src/popup/staking/solo-new/settings/ChooseAccount.tsx (1)
60-60: Consider using a stable identifier for the Fragment key.The Fragment currently uses
indexas the key, which can cause issues if the accounts list is reordered or filtered. Consider usingaddressorformattedinstead, as they provide stable unique identifiers.Apply this diff:
- <Fragment key={index}> + <Fragment key={address}> <Container disableGutters onClick={handleSelect(formatted)} sx={{ alignItems: 'center', cursor: 'pointer', display: 'flex', flexDirection: 'row', justifyContent: 'space-between' }}>packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (1)
168-186: Recommended: Always pass onImport to avoid race conditions.Conditionally passing
undefinedtoonSetPasswordbased on parent state can race with child component updates. If the user presses Enter while the parent hasn't updated, the action may silently no-op.Let
MatchPasswordFieldgate the action based on its own validation state:- {localAccounts?.length === 0 - ? (<MatchPasswordField - onSetPassword={(password && name && !error && !!seed) ? onImport : undefined} + {localAccounts?.length === 0 + ? (<MatchPasswordField + onSetPassword={onImport} setConfirmedPassword={setPassword} spacing='20px'packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (2)
88-90: Avoid casting Promises to void; handle rejections explicitlyReplace
as unknown as voidwith a safe fire‑and‑forget pattern.- hasLocalAccounts && setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + if (hasLocalAccounts) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error); + } + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); @@ - setHashedPassword(undefined); + setHashedPassword(undefined); + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error);Also applies to: 98-101
103-106: Prefervoid navigate(path)overas voidcastMinor readability/style improvement.
- isExtension - ? windowOpen(path).catch(console.error) - : navigate(path) as void; + isExtension + ? windowOpen(path).catch(console.error) + : void navigate(path);packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (2)
99-108: Clear sensitive state on completionUnset
masterPass/currentPasswordin memory once migration is finalized.const onDone = useCallback(() => { setIsBusy(true); setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) .then(() => { setExtensionLock(true); - navigate('/') as void; + navigate('/') as void; lockExtension().catch(console.error); - }).catch(console.error); + }).catch(console.error) + .finally(() => { + // minimize secret lifetime + // eslint-disable-next-line no-unused-expressions + setCurrentPass(''); + // eslint-disable-next-line no-unused-expressions + // masterPass is used only for this flow; clear it + // @ts-expect-error: allowed for secret clearing + setMasterPass(''); + }); }, [navigate, setExtensionLock]);
70-97: EnsureisBusyresets in all pathsUse
finallyto guaranteesetIsBusy(false)regardless of success/error/early exit.const onNext = useCallback(() => { if (accountToMigrate && masterPass) { - setIsBusy(true); - accountsChangePassword(accountToMigrate.address, currentPassword, masterPass) - .then((success) => { + setIsBusy(true); + accountsChangePassword(accountToMigrate.address, currentPassword, masterPass) + .then((success) => { if (success) { if (accountIndexToChangePassword + 1 === accountsNeedMigration?.length) { - setIsBusy(false); - - return setStep(STEP.COMPLETED); + setStep(STEP.COMPLETED); + return; } setAccountIndex((pre) => pre + 1); setCurrentPass(''); } else { notify(t('Something went wrong while migrating password!'), 'error'); } - setIsBusy(false); }).catch((error) => { console.error(error); setIncorrectPassword(true); - setIsBusy(false); - }); + }).finally(() => setIsBusy(false)); } else { - setIsBusy(false); + setIsBusy(false); } }, [accountIndexToChangePassword, accountToMigrate, accountsNeedMigration?.length, currentPassword, masterPass, notify, t]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/extension-base/src/background/handlers/Extension.ts(9 hunks)packages/extension-base/src/background/types.ts(5 hunks)packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx(2 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsForgotten.ts(1 hunks)packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension-polkagate/src/popup/staking/solo-new/settings/ChooseAccount.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (10)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-46)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(21-26)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(100-102)windowOpen(263-265)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(72-85)updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
LOGIN_STATUS(4-11)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (4)
packages/extension-base/src/background/handlers/Extension.ts (2)
localAccounts(422-426)accountsValidate(240-248)packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (3)
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)packages/extension-polkagate/src/components/Loading.tsx (2)
watchStorage(51-70)getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)
packages/extension-base/src/background/types.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (7)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-46)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(102-120)lockExtension(145-180)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)
packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (5)
packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)packages/extension-polkagate/src/messaging.ts (1)
validateSeed(251-253)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (1)
useAccountImportOrCreate(19-112)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(236-239)RequestUnlockAllAccounts(361-364)RequestSigningSignature(319-321)RequestUpdateMeta(356-359)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
[error] 54-54: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/extension-polkagate/src/popup/staking/solo-new/settings/ChooseAccount.tsx (1)
61-61: LGTM: Correct removal of redundant key.Removing the
keyprop from theContaineris appropriate since the parentFragment(line 60) already provides the key for the list item.packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx (1)
52-76: Duplicate key issue resolved - LGTM!The previous review flagged a duplicate
key={address}prop on both the outer div and inner Container. The current implementation correctly places the key only on the outermostdivelement (line 52), with the Container (line 53) no longer having a duplicate key. This follows React best practices for list rendering.packages/extension-base/src/background/handlers/Extension.ts (1)
192-196: LGTM: removing cached unlock on forgetStale cache entry is deleted before forgetting the account.
… after tx submission
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/components/SignArea3.tsx (1)
100-129: Critical: signer payload builds Call from the whole extrinsic (invalid) and hardcodes extensions.
methodmust be the call, not the full extrinsic. Using the extrinsic will produce an invalid payload/signature.- Prefer
api.registry.signedExtensionsover a hardcoded list to match the runtime and avoid silent mismatches.- Prefer
api.extrinsicVersionas the payload version source of truth.- Optional but safer: provide
specVersionandtransactionVersionexplicitly instead ofruntimeVersion.Apply this minimal fix first (correct Call + version):
- method: api.createType('Call', preparedTransaction), // TODO: DOES SUPPORT nested calls, batches , ... + // Use the call only; supports nested/batch calls via .method + method: preparedTransaction.method, @@ - signedExtensions: [ - 'CheckNonZeroSender', - 'CheckSpecVersion', - 'CheckTxVersion', - 'CheckGenesis', - 'CheckMortality', - 'CheckNonce', - 'CheckWeight', - 'ChargeTransactionPayment' - ], + // Derive from registry to match the chain at runtime + signedExtensions: api.registry.signedExtensions, @@ - version: preparedTransaction.version + version: api.extrinsicVersionOptional robustness (if you prefer explicit versions):
- runtimeVersion: api.runtimeVersion, + specVersion: api.runtimeVersion.specVersion, + transactionVersion: api.runtimeVersion.transactionVersion,
♻️ Duplicate comments (3)
packages/extension-polkagate/src/util/api/signAndSend.ts (3)
50-53: Unify transactionState event detail shape and guard for non-window contextsEmit a consistent detail object (e.g., { state }) and guard for environments without window (bg service worker).
- const event = new CustomEvent('transactionState', { detail: result.status.type }); - - window.dispatchEvent(event); + const detail = { state: result.status.type }; + if (typeof window !== 'undefined' && typeof window.dispatchEvent === 'function') { + window.dispatchEvent(new CustomEvent('transactionState', { detail })); + }Verification (grep listeners expecting a string vs object):
#!/bin/bash rg -nP "addEventListener\\(.*transactionState|on\\(['\"]transactionState['\"]" -A3 -B3
122-128: Capture and dispose unsubscribe from signAndSend to prevent leaksThe callback variant returns an unsubscribe function; not capturing it leaks the subscription. Mirror the send() pattern.
- extrinsic.signAndSend(pair, options ?? {}, async (result) => { - await handleResult(api, resolve, result); - }).catch((e) => { + let unsub: (() => void) | undefined; + const onResult = async (result: ISubmittableResult) => { + const { status } = result; + await handleResult(api, resolve, result); + if ( + status.isInBlock || + status.isFinalized || + status.isInvalid || + status.isDropped || + status.isUsurped || + status.isRetracted || + status.isFinalityTimeout + ) { + unsub?.(); + } + }; + extrinsic.signAndSend(pair, options ?? {}, onResult).then((u) => { unsub = u; }).catch((e) => { console.log('⚠️ Catch error', e); resolve({ block: 0, failureText: String(e), fee: '', success: false, txHash: '' }); - }); + unsub?.(); + });
79-104: Handle Retracted and always return after resolving terminal statesRetracted is a terminal failure and should resolve. Also add an early return after the failure resolve to prevent accidental double-resolve if this function grows.
- try { - const { isCompleted, isError, status, txHash } = result; - const { isDropped, isFinalityTimeout, isFinalized, isInBlock, isInvalid, isUsurped, type } = status; + try { + const { isCompleted, isError, status, txHash } = result; + const { isDropped, isFinalityTimeout, isFinalized, isInBlock, isInvalid, isUsurped, isRetracted, type } = status; @@ - if (isCompleted && isError) { + if (isCompleted && isError) { resolve({ block: 0, failureText: failureText || `unknown error (${type})`, fee: '', success: false, txHash: '' }); - - return; + return; } @@ - if (isInvalid || isDropped || isUsurped || isFinalityTimeout) { - resolve({ block: 0, failureText: failureText || status.type, fee: '', success: false, txHash: txHash?.toString() || '' }); - } + if (isInvalid || isDropped || isUsurped || isRetracted || isFinalityTimeout) { + resolve({ block: 0, failureText: failureText || status.type, fee: '', success: false, txHash: txHash?.toString() || '' }); + return; + }
🧹 Nitpick comments (5)
packages/extension-polkagate/src/components/SignArea3.tsx (1)
209-223: Result mapping improvements are fine; consider defensive defaults.Destructuring with defaults for
blockandtxHashis OK. Ifdecimal/tokencan vary across chains, ensure UI formatting tolerates''or0gracefully.packages/extension-polkagate/src/util/api/signAndSend.ts (4)
8-12: Drop ts-ignore and use stable EventRecord typingAvoid importing lookup-specific FrameSystemEventRecord with a ts-ignore. Prefer EventRecord from interfaces and update the Vec generic accordingly.
import type { Vec } from '@polkadot/types'; -import type { SignedBlock } from '@polkadot/types/interfaces'; -// @ts-ignore -import type { FrameSystemEventRecord } from '@polkadot/types/lookup'; +import type { SignedBlock } from '@polkadot/types/interfaces'; +import type { EventRecord } from '@polkadot/types/interfaces'; @@ - const allEvents = await apiAt.query['system']['events']() as Vec<FrameSystemEventRecord>; + const allEvents = await apiAt.query.system.events() as Vec<EventRecord>;Also applies to: 18-18
16-24: Tx hash param typing: accept string to match toHex() and avoid TS frictionresult.txHash.toHex() is typed as string; your function expects HexString. Broaden the param to string (or HexString | string) to keep call-sites clean.
-async function getAppliedFee (api: ApiPromise, signedBlock: SignedBlock, txHashHex: HexString): Promise<string | undefined> { +async function getAppliedFee (api: ApiPromise, signedBlock: SignedBlock, txHashHex: string): Promise<string | undefined> {Also applies to: 29-41
56-70: Include section.name in failureText for better UXToday only docs are surfaced. Append section.name for clearer, actionable error messages.
- failureText = `${docs.join(' ')}`; + failureText = `${section}.${name}: ${docs.join(' ')}`.trim();
52-54: Reduce noisy console logs or gate by envThese console logs are verbose for production. Consider gating behind a debug flag.
Also applies to: 120-121, 139-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension-polkagate/src/components/SignArea3.tsx(9 hunks)packages/extension-polkagate/src/fullscreen/sendFund/index.tsx(1 hunks)packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(131-172)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (6)
packages/extension-polkagate/src/fullscreen/sendFund/index.tsx (1)
1-271: No issues found. Crypto initialization is properly centralized.The removal of local
cryptoWaitReady()from the SendFund component is correct. The crypto initialization has been properly moved topackages/extension/src/background.ts(lines 32–42), where it runs as part of the extension's service worker initialization on startup. This guarantees that crypto is ready before any UI components, including SignArea3, attempt to sign transactions. No race conditions exist because the background script initializes before the UI becomes interactive.packages/extension-polkagate/src/components/SignArea3.tsx (5)
72-74: Hook usage update looks correct.Switching to
useProxies(genesisHash, formatted)aligns with the hook’s signature and avoids coupling toapi.
125-129: Good: using SignerPayload.toPayload().Converting to the JSON payload before constructing
ExtrinsicPayloadis correct and interoperable with external signers.
137-144: LGTM: ExtrinsicPayload creation.
api.registry.createType('ExtrinsicPayload', signerPayload, { version: signerPayload.version })is the right shape foraddSignature(...).
264-269: Prop shape change topayload={extrinsicPayload}looks consistent across Ledger and QR.Assuming downstream components consume
ExtrinsicPayload, these changes are coherent with the new signing flow.If either component still expects the older
payloadshape, update their prop types accordingly. The script above will surface any mismatches.Also applies to: 300-303
231-243: Signature flow and payload types verified as correct.Both
SignWithLedgerandSignUsingQRexpectpayload: GenericExtrinsicPayload | undefined, and SignArea3.tsx correctly passesextrinsicPayloadto both components at lines 264 and 300. The code is properly typed and children receive the correct payload type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/components/SignArea3.tsx (1)
100-129: Fix SignerPayload construction (missing spec/tx version, wrong method, hard-coded extensions).Current payload is not runtime-accurate and may yield invalid/malleated signatures:
- specVersion/transactionVersion are missing; runtimeVersion is not a SignerPayload field.
- method should be the call (tx.method), not the whole extrinsic.
- signedExtensions must come from registry; hard-coding omits chain-specific items (e.g., CheckMetadataHash, ChargeAssetTxPayment), breaking signature on many networks.
- tip should be Compact.
Apply:
const _payload = { address: from, - assetId: signerOption?.assetId, - blockHash: lastHeader.hash, - blockNumber: api.registry.createType('BlockNumber', lastHeader.number.toNumber()), - era: api.registry.createType('ExtrinsicEra', { current: lastHeader.number.toNumber(), period: 64 }), - genesisHash: api.genesisHash, - method: api.createType('Call', preparedTransaction), // TODO: DOES SUPPORT nested calls, batches , ... - nonce: api.registry.createType('Compact<Index>', rawNonce), - runtimeVersion: api.runtimeVersion, - signedExtensions: [ - 'CheckNonZeroSender', - 'CheckSpecVersion', - 'CheckTxVersion', - 'CheckGenesis', - 'CheckMortality', - 'CheckNonce', - 'CheckWeight', - 'ChargeTransactionPayment' - ], - tip: 0, - version: preparedTransaction.version + assetId: signerOption?.assetId, + blockHash: lastHeader.hash, + blockNumber: api.registry.createType('BlockNumber', lastHeader.number.toNumber()), + era: api.registry.createType('ExtrinsicEra', { current: lastHeader.number.toNumber(), period: 64 }), + genesisHash: api.genesisHash, + method: preparedTransaction.method, + nonce: api.registry.createType('Compact<Index>', rawNonce), + // Pull from runtime to guarantee match + specVersion: api.runtimeVersion.specVersion.toNumber(), + transactionVersion: api.runtimeVersion.transactionVersion.toNumber(), + // Ensure asset-fee extension when assetId is present + signedExtensions: signerOption?.assetId + ? Array.from(new Set([...api.registry.signedExtensions, 'ChargeAssetTxPayment'])) + : api.registry.signedExtensions, + tip: api.registry.createType('Compact<Balance>', signerOption?.tip ?? 0), + version: preparedTransaction.version }; - const raw = api.registry.createType('SignerPayload', _payload, { - version: _payload.version - }); + const raw = api.registry.createType('SignerPayload', _payload, { version: _payload.version }); return raw.toPayload();Optionally derive era/nonce using chain derive for better liveness:
- era/current from latest header subscription
- nonce from api.derive.tx.signingInfo(from)
Also applies to: 135-135
🧹 Nitpick comments (2)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
209-222: TxResult mapping LGTM (new shape).Fields align with util/api/send() return; ensure fee inclusion later if needed by UI.
231-243: Signature flow OK; add minimal failure handling.Consider catching send() rejection to revert WAIT state if message port drops.
- const txResult = await send(from, api, preparedTransaction, extrinsicPayload.toHex(), signature); + let txResult: TxResult; + try { + txResult = await send(from, api, preparedTransaction, extrinsicPayload.toHex(), signature); + } catch (e) { + setFlowStep(TRANSACTION_FLOW_STEPS.REVIEW); // or an error step + throw e; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/components/SignArea3.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(131-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/extension-polkagate/src/components/SignArea3.tsx (6)
9-9: Import looks fine.Type usage aligns with SubmittableExtrinsic typing in this file.
73-74: useProxies signature update LGTM.Passing (genesisHash, formatted) matches the updated hook contract.
137-144: ExtrinsicPayload creation looks correct.Creating from payload with matching version is the right path.
264-269: Props are correctly typed and separated.The verification confirms that SignArea3.tsx correctly passes
extrinsicPayload(type: GenericExtrinsicPayload) to thepayloadprop, while separately passingsignerPayload(type: SignerPayloadJSON) to thesignerPayloadprop. TheextrinsicPayloadis properly created viaapi.registry.createType('ExtrinsicPayload', ...), which returns the expected GenericExtrinsicPayload type that SignWithLedger expects.
278-284: Incorrect analysis - all signers consistently use SignerPayloadJSON.The review comment mischaracterizes the payload usage:
- SignWithLedger receives both
payload: GenericExtrinsicPayloadandsignerPayload: SignerPayloadJSON- SignUsingQR receives both
payload: GenericExtrinsicPayloadandsignerPayload: SignerPayloadJSON- SignUsingPassword receives
signerPayload: SignerPayloadJSONAll three signers consistently use
SignerPayloadJSON. The additionalGenericExtrinsicPayloadin Ledger and QR is intentional—it's needed to display the transaction details to the user for verification before signing. Password signing doesn't require this because it doesn't display extrinsic details to the user. This is a sensible design distinction, not an inconsistency requiring unification.Likely an incorrect or invalid review comment.
300-303: QR flow prop handling is correct; verification complete.The extrinsicPayload is properly created as an
ExtrinsicPayloadviaapi.registry.createType()and includes signedExtensions through the signerPayload context. The type casts betweenGenericExtrinsicPayloadandExtrinsicPayloadare appropriate, and the Qr component correctly receives and processes the payload with.toU8a().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/extension-polkagate/src/partials/WaitScreen.tsx (1)
57-57: Consider responsive sizing for the animation.The animation uses a fixed width of
300px, which might not scale well across different viewport sizes or devices.Consider using responsive units or percentage-based sizing:
- <DotLottieReact autoplay loop src={sendingLottie} style={{ height: 'auto', width: '300px' }} /> + <DotLottieReact autoplay loop src={sendingLottie} style={{ height: 'auto', width: '100%', maxWidth: '300px' }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension-polkagate/src/assets/animations/index.ts(1 hunks)packages/extension-polkagate/src/partials/WaitScreen.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (4)
packages/extension-polkagate/src/assets/animations/index.ts (1)
6-9: LGTM! Animation exports align with PR objectives.The exported animations (lockLottie, masterKey, migratePassword, sendingLottie) are properly structured and align with the master password migration feature being added in this PR.
packages/extension-polkagate/src/partials/WaitScreen.tsx (3)
4-4: LGTM! Lottie animation imports are properly structured.The imports for
DotLottieReactandsendingLottiecorrectly reference the new animation infrastructure added in this PR.Also applies to: 10-11
33-46: LGTM! State normalization and default case handling are well-implemented.The
toLowerCase()normalization handles case variations in transaction states, and the default case provides good fallback behavior for unknown states.
56-56: LGTM! Layout adjustments accommodate the new animation.The styling changes appropriately adjust spacing and layout to accommodate the DotLottie animation replacement.
…low (#1978) * fix: gracefully handle non-existent key pair during forgot password flow * fix: ensure lockExtension handles cached unlocks safely when pairs no longer exist
…login when expired (#1980) * feat: add proactive unlock cache expiry detection and redirect UI to login when expired * refactor: use one global unlockExpiry for all accounts * chore: fix conflict issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
44-50: Do not skip the lock-expiry check when migration is pending.This issue has already been identified in a previous review. The lock-expiry check currently only runs when
isPasswordsMigratedis truthy, which excludes non-migrated users (whereisPasswordsMigrated === false). The check should run for both migrated and non-migrated users, gating only on whether the migration state is known (isPasswordsMigrated !== undefined).packages/extension-base/src/background/handlers/Extension.ts (4)
132-140: Handle individual account password-change failures gracefully.If
accountsChangePasswordthrows for any account (e.g., corrupted keystore, internal error), the entire operation aborts and no accounts are updated. Wrap each call in a try-catch to handle failures per account and return partial success information.Apply this diff:
private accountsChangePasswordAll ({ newPass, oldPass }: RequestAccountChangePasswordAll): boolean { const accountsLocal = this.localAccounts(); const res = accountsLocal.map(({ address }) => { - return this.accountsChangePassword({ address, newPass, oldPass }); + try { + return this.accountsChangePassword({ address, newPass, oldPass }); + } catch (error) { + console.error(`Failed to change password for account ${address}:`, error); + return false; + } }); return res.every(Boolean); }
517-531: Avoid global TypeRegistry mutation; create a per-request registry for signing.Global registry is shared across concurrent requests and may be reconfigured mid-sign, causing non-deterministic behavior. Build and use a local registry per payload.
Apply this diff to create a per-request registry:
-private handleRegistry (payload: SignerPayloadJSON): void { +private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); // Get the metadata for the genesisHash const currentMetadata = this.metadataGet(payload.genesisHash); // set the registry before calling the sign function const signedExtensions = currentMetadata?.signedExtensions?.length ? currentMetadata.signedExtensions - : registry.signedExtensions; + : reg.signedExtensions; - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - if (currentMetadata) { - registry.register(currentMetadata?.types); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); } + + return reg; }Then update call sites (Line 561 and Line 591) to use the returned registry.
576-595: Harden getSignature: fix validation order, handle missing accounts, and use local registry.Multiple issues in this method:
- Line 577 validates
payloadbut Line 581 destructuresaddressbefore the validation check. Ifpayloadis null/undefined, Line 581 throws before validation.- Line 583:
keyring.getPair(address)can throw for unknown/forgotten accounts. Return null instead of propagating the error.- Lines 591-592: Uses global registry via
handleRegistrywhich can cause race conditions with concurrent requests.Apply this diff to fix all issues:
private getSignature ({ payload }: RequestSigningSignature): HexString | null { if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } const { address } = payload; - const pair = keyring.getPair(address); + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); return signature; }Note: This assumes
handleRegistryis refactored tocreateSigningRegistryas suggested in the previous comment.
561-564: Update to use per-request registry from refactored method.Once
handleRegistryis refactored tocreateSigningRegistry(returning a local registry), update this call site to capture and use the returned registry for signing instead of the global instance.Example:
if (isJsonPayload(payload)) { - this.handleRegistry(payload); + const reg = this.createSigningRegistry(payload); + const result = request.sign(reg, pair); +} else { + const result = request.sign(registry, pair); } - -const result = request.sign(registry, pair);Note: Verify that
request.signaccepts a registry parameter; if not, you may need to adjust the signing flow to use the local registry.
🧹 Nitpick comments (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
32-42: Improve type safety for the message listener.The message parameter is typed as
any, which bypasses type checking. Consider defining a proper message type.Apply this diff to add type safety:
+interface LockExpiredMessage { + type: 'LOCKED_ACCOUNTS_EXPIRED'; +} + useEffect(() => { - const handleLockExpiredMessage = (msg: any) => { + const handleLockExpiredMessage = (msg: LockExpiredMessage) => { if (msg.type === 'LOCKED_ACCOUNTS_EXPIRED') { window.location.reload(); } };Additionally, verify the hard reload behavior.
window.location.reload()discards all application state. Confirm this is the intended behavior rather than a softer state reset through React context updates.packages/extension-base/src/background/handlers/Extension.ts (1)
478-511: Consider handling individual account unlock failures gracefully.The current implementation wraps the entire unlock operation in a single try-catch. If
decodePkcs8fails for any account (e.g., corrupted keystore), the operation returns false and the global expiry is not set for any accounts. Consider wrapping each account unlock in a try-catch to allow partial success, where successfully unlocked accounts still get the benefit of the cache time.Example approach:
private accountsUnlockAll ({ cacheTime, password }: RequestUnlockAllAccounts): boolean { if (!password) { throw new Error('Password needed to unlock the account'); } - try { - const accountsLocal = this.localAccounts(); - - for (const { address } of accountsLocal) { + const accountsLocal = this.localAccounts(); + const results: boolean[] = []; + + for (const { address } of accountsLocal) { + try { const pair = keyring.getPair(address); if (!pair) { - throw new Error('Unable to find pair'); + results.push(false); + continue; } if (!pair.isLocked) { pair.lock(); } if (pair.isLocked) { pair.decodePkcs8(password); } + + results.push(true); + } catch (error) { + console.error(`Failed to unlock account ${address}:`, error); + results.push(false); } + } + if (results.some(Boolean)) { // Set a single expiry timestamp for all accounts this.setUnlockExpiry(Date.now() + cacheTime); + } - return true; - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + return results.every(Boolean); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension-base/src/background/handlers/Extension.ts(10 hunks)packages/extension-polkagate/src/context/ExtensionLockContext.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (2)
packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/messaging.ts (1)
areAccountsLocksExpired(104-106)
packages/extension-base/src/background/handlers/Extension.ts (2)
packages/extension-base/src/background/handlers/State.ts (1)
State(134-584)packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(237-240)RequestUnlockAllAccounts(362-365)RequestSigningSignature(320-322)RequestUpdateMeta(357-360)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
4-4: LGTM: Imports and hook integration are correct.The new imports and hook usage properly support the lock-expiry and migration workflows.
Also applies to: 6-7, 27-27
packages/extension-base/src/background/handlers/Extension.ts (1)
62-64: ****A listener for the
LOCKED_ACCOUNTS_EXPIREDmessage type is already properly configured in the codebase. Inpackages/extension-polkagate/src/context/ExtensionLockContext.tsx, the listener is registered viachrome.runtime.onMessage.addListener()at line 39 to handle this exact message type (line 34), triggering a window reload when received. The message will be successfully delivered and handled without the.catch(console.error)being invoked for a missing listener.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/extension-polkagate/src/hooks/usePool.ts(1 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)packages/extension/public/locales/en/translation.json(1 hunks)packages/extension/public/locales/es/translation.json(1 hunks)packages/extension/public/locales/fr/translation.json(1 hunks)packages/extension/public/locales/hi/translation.json(2 hunks)packages/extension/public/locales/ru/translation.json(1 hunks)packages/extension/public/locales/zh/translation.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/hooks/usePool.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/public/locales/hi/translation.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (6)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-40)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
resetOnForgotPassword(10-18)packages/extension-polkagate/src/messaging.ts (1)
createAccountSuri(173-175)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/util/constants.ts (2)
STORAGE_KEY(309-330)PROFILE_TAGS(260-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
packages/extension/public/locales/zh/translation.json (1)
852-872: Translations for master password feature are complete and properly formatted.The new translations for master password functionality (setup, migration, auto-lock, password management, and account derivation) are well-integrated into the locale file. All placeholder variables (
{{name}},{{migrated}},{{count}}) are preserved correctly, and the Chinese translations are consistent with the feature semantics.Minor observation: Line 852 ("Missing tokens? Manage Networks!") is marked with
~but the translation appears unchanged from the original. Verify this is intentional or a marker artifact.packages/extension/public/locales/ru/translation.json (1)
858-878: Master password translation strings are properly formatted and consistent.The 20 new Russian translations for the master password feature follow existing conventions:
- All template variables (
{{name}},{{migrated}},{{count}}) are preserved correctly- JSON structure is valid with no syntax errors
- Terminology (e.g., "мастер-пароль", "учетная запись", "миграция") is consistent with other translation entries
- Error and completion messages follow the established pattern
packages/extension/public/locales/fr/translation.json (1)
860-879: Master password workflow translations are well-formatted and consistent.The new French translations for the master password migration feature are properly structured with correct JSON syntax, preserved template variables (
{{name}},{{migrated}},{{count}}), and French localization conventions. All entries maintain consistency with existing translation patterns in the file.packages/extension/public/locales/es/translation.json (1)
1-880: Translation file is well-structured and complete.The JSON is syntactically valid, and the Spanish translations follow consistent terminology, proper grammar, and appropriate formality level throughout. The ~20 new master-password related entries are comprehensive and properly formatted with correct placeholder interpolation.
To ensure completeness, please verify that all new translation keys added in lines 860–879 are actually referenced in the master-password UI code. For example, keys like
"Set master password","Migrate accounts to master password", and the long-form descriptions should have corresponding usage in components handling master-password flows (e.g., migration dialogs, password setup screens).packages/extension/public/locales/en/translation.json (4)
804-804: Clarify placement of existing key refresh.Line 804 appears to be an existing key ("Missing tokens? Manage Networks!") that's now being included in the master-password translation set. Verify that this repositioning is intentional and not accidentally mixing context boundaries between feature sections.
822-822: Verify contextual appropriateness of this key within master-password feature.The phrase "This child account will be created from your parent account. Give it a name." relates to derived account creation, not master-password migration or unlock flows. Confirm this key belongs in the master-password feature translations or if it should be in a separate account-derivation section.
808-808: Ensure placeholder variable names align with actual template context.Lines 808, 816, and 817 use placeholders
{{name}},{{migrated}}, and{{count}}. Verify these placeholders are correctly supplied by the code consuming these translation strings, and that the surrounding UI correctly displays these values in context (e.g., "Migrate account 2 of 5").Also applies to: 816-817
814-818: Review lengthy onboarding/migration descriptions for clarity and tone.These multi-sentence descriptions (lines 814-815, 818) provide good context for users but are quite long. Verify that:
- They fit within intended UI layouts without truncation
- The tone and terminology ("master password," "unlock and sign") are consistent with the rest of the application
- They are tested with actual UI rendering to confirm readability
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Show resolved
Hide resolved
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Outdated
Show resolved
Hide resolved
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/extension-polkagate/src/hooks/useAutoLock.ts (1)
27-29: Optional: initialize state with DEFAULT_AUTO_LOCK to remove undefined states.Prevents UI flicker and repeated optional chaining.
-export default function useAutoLock (): AutoLock | undefined { - const [autoLock, setAutoLock] = useState<AutoLock>(); +export default function useAutoLock (): AutoLock { + const [autoLock, setAutoLock] = useState<AutoLock>(DEFAULT_AUTO_LOCK);packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts (1)
4-8: Type the option values and keep labels i18n‑friendly.
- Tie value to AutoLockDelayType to prevent drift.
- Prefer translating labels at render time rather than hardcoding.
+import type { AutoLockDelayType } from '@polkadot/extension-polkagate/src/hooks/useAutoLock'; - -export const autoLockOptions = [ - { text: 'min', value: 'min' }, - { text: 'hour', value: 'hour' }, - { text: 'day', value: 'day' } -]; +export const autoLockOptions: { text: string; value: AutoLockDelayType }[] = [ + { text: 'min', value: 'min' }, + { text: 'hour', value: 'hour' }, + { text: 'day', value: 'day' } +];Confirm whether DropSelect expects pre‑translated text, or if we should pass translation keys and call t() in the consumer.
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
47-47: Prefer nullish coalescing once input is validated/clamped in the hook.After clamping in the hook, this avoids surprising fallback when the user types 0 while we surface a clear min constraint.
- inputValue={inputValue || autoLock?.delay?.value || AUTO_LOCK_PERIOD_DEFAULT} + inputValue={inputValue ?? autoLock?.delay?.value ?? AUTO_LOCK_PERIOD_DEFAULT}packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
63-68: Optional: avoid JSON.stringify for deep equality.Stringify is O(n) and brittle. Consider shallow compare of fields or a tiny deepEqual util.
- if (!autoLock || JSON.stringify(toSave) === JSON.stringify(autoLock)) { + if ( + !autoLock || + (toSave.enabled === autoLock.enabled && + toSave.delay.type === autoLock.delay.type && + toSave.delay.value === autoLock.delay.value) + ) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/extension-polkagate/src/fullscreen/settings/ExtensionSettings.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx(0 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx(1 hunks)packages/extension-polkagate/src/hooks/useAutoLock.ts(2 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/Main.tsx(2 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx(0 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx
- packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
packages/extension-polkagate/src/popup/settings/extensionSettings/Main.tsx (1)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
AutoLockTimerAdjustment(15-68)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (5)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
useAdjustAutoLockTimer(17-78)packages/extension-polkagate/src/util/constants.ts (1)
AUTO_LOCK_PERIOD_DEFAULT(226-226)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts (1)
autoLockOptions(4-8)
packages/extension-polkagate/src/hooks/useAutoLock.ts (1)
packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
packages/extension-polkagate/src/fullscreen/settings/ExtensionSettings.tsx (1)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
AutoLockTimerAdjustment(15-68)
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (2)
packages/extension-polkagate/src/hooks/useAutoLock.ts (3)
AutoLock(11-17)AutoLockDelayType(9-9)useAutoLock(27-39)packages/extension-polkagate/src/util/constants.ts (2)
AUTO_LOCK_PERIOD_DEFAULT(226-226)STORAGE_KEY(309-330)
🔇 Additional comments (3)
packages/extension-polkagate/src/fullscreen/settings/ExtensionSettings.tsx (1)
11-11: Replacement to AutoLockTimerAdjustment looks correct.Import path and placement are consistent with the new flow.
Also applies to: 37-37
packages/extension-polkagate/src/popup/settings/extensionSettings/Main.tsx (1)
7-7: LGTM on swapping in AutoLockTimerAdjustment.No behavioral concerns.
Also applies to: 18-18
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
11-11: Fix import source for setStorage.The
setStoragefunction is exported from@polkadot/extension-polkagate/src/util(viautil/storage.ts), notcomponents/Loading. Using the wrong source risks circular dependencies and inconsistency across the codebase where 11 other files correctly import from util.-import { setStorage } from '../../../components/Loading'; +import { setStorage } from '@polkadot/extension-polkagate/src/util';
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/extension-base/src/background/handlers/Extension.ts (4)
132-140: Handle individual account password-change failures gracefully.If
accountsChangePasswordthrows for any account, the entire operation fails and no accounts are updated. The past review comment on this issue remains unaddressed.Apply this diff to handle failures per account:
private accountsChangePasswordAll ({ newPass, oldPass }: RequestAccountChangePasswordAll): boolean { const accountsLocal = this.localAccounts(); const res = accountsLocal.map(({ address }) => { - return this.accountsChangePassword({ address, newPass, oldPass }); + try { + return this.accountsChangePassword({ address, newPass, oldPass }); + } catch (error) { + console.error(`Failed to change password for account ${address}:`, error); + return false; + } }); return res.every(Boolean); }
244-252: Handle individual account forget failures gracefully.If
accountsForgetthrows for any account, the entire operation fails and remaining accounts are not processed. The past review comment on this issue remains unaddressed.Apply this diff to handle failures per account:
private accountsForgetAll (): boolean { const accounts = keyring.getAccounts(); accounts.forEach(({ address }) => { - this.accountsForget({ address }); + try { + this.accountsForget({ address }); + } catch (error) { + console.error(`Failed to forget account ${address}:`, error); + } }); return true; }
517-531: Avoid global TypeRegistry mutation; create a per-request registry for signing.The global
registryis shared across concurrent requests and may be reconfigured mid-sign, causing non-deterministic behavior. The past review comment on this issue remains unaddressed.Refactor to create and return a per-request registry:
-private handleRegistry (payload: SignerPayloadJSON): void { +private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); const currentMetadata = this.metadataGet(payload.genesisHash); const signedExtensions = currentMetadata?.signedExtensions?.length ? currentMetadata.signedExtensions - : registry.signedExtensions; + : reg.signedExtensions; - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - if (currentMetadata) { - registry.register(currentMetadata?.types); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); } + + return reg; }Then update call sites (lines 561, 591) to use the returned registry.
576-595: Handle unknown accounts gracefully and avoid global registry mutation.While the payload validation issue has been fixed, two issues remain:
Missing error handling:
keyring.getPair(address)at Line 583 can throw for unknown or forgotten accounts, causing the method to reject instead of returningnullas intended.Global registry mutation: Line 591 calls
handleRegistry(payload)which mutates the shared global registry, causing potential race conditions between concurrent signing requests.Apply this diff to handle unknown accounts:
private getSignature ({ payload }: RequestSigningSignature): HexString | null { if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } const { address } = payload; - const pair = keyring.getPair(address); + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } this.refreshAccountPasswordCache(pair); if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); return signature; }Note: This assumes
handleRegistryis refactored tocreateSigningRegistryas suggested in the earlier comment.
🧹 Nitpick comments (2)
packages/extension-base/src/background/handlers/Extension.ts (2)
478-511: Consider handling individual account unlock failures gracefully.The current implementation uses a single try-catch around the entire loop. If any account fails to unlock (e.g., wrong password for one account, corrupted keystore), the operation returns
falseand no accounts are unlocked. This all-or-nothing approach may not provide the best user experience.Consider tracking which accounts succeeded and which failed, then returning a more informative result (or at least logging failures per account while continuing to process remaining accounts).
Example approach:
private accountsUnlockAll ({ cacheTime, password }: RequestUnlockAllAccounts): boolean { if (!password) { throw new Error('Password needed to unlock the account'); } - try { - const accountsLocal = this.localAccounts(); - - for (const { address } of accountsLocal) { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (!pair.isLocked) { - pair.lock(); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - } - - // Set a single expiry timestamp for all accounts - this.setUnlockExpiry(Date.now() + cacheTime); - - return true; - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + const accountsLocal = this.localAccounts(); + let successCount = 0; + + for (const { address } of accountsLocal) { + try { + const pair = keyring.getPair(address); + + if (!pair) { + console.error(`Unable to find pair for account ${address}`); + continue; + } + + if (pair.isLocked) { + pair.decodePkcs8(password); + } + + successCount++; + } catch (error) { + console.error(`Failed to unlock account ${address}:`, error); + } + } + + if (successCount > 0) { + // Set expiry if at least one account was unlocked + this.setUnlockExpiry(Date.now() + cacheTime); + return true; + } + + return false; }
513-515: Clarify the semantics ofareLocksExpiredwhen no expiry is set.The method returns
truewhen#unlockExpiry === null, which means "no unlock is active" rather than "locks have expired". This might be semantically confusing for callers expecting a distinction between "never unlocked" and "expired".Consider whether the method name and return value accurately represent the intended state. If
nullmeans "all accounts are locked" (which is effectively "expired" from an unlock perspective), the current logic is correct but could benefit from a clarifying comment.Example clarification:
private areLocksExpired (): boolean { + // Returns true if no unlock is active or if the unlock has expired return this.#unlockExpiry === null || this.#unlockExpiry < Date.now(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-base/src/background/handlers/Extension.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-base/src/background/handlers/Extension.ts (2)
packages/extension-base/src/background/handlers/State.ts (1)
State(134-584)packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(237-240)RequestUnlockAllAccounts(362-365)RequestSigningSignature(320-322)RequestUpdateMeta(357-360)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/extension-base/src/background/handlers/Extension.ts (1)
186-226: LGTM! Past issues have been addressed.The method now correctly:
- Iterates local accounts using
for...ofinstead ofObject.entries- Wraps
getPairin try-catch to handle forgotten accounts gracefully- Uses
withErrorLogwrapper forchrome.tabs.reload- Clears unlock expiry before reloading tabs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (2)
69-69: Check the boolean result of resetOnForgotPassword.The function returns a boolean indicating success or failure, but the result is ignored. If the reset fails, account creation proceeds anyway, potentially leaving forgotten accounts in an inconsistent state.
Apply this diff to check the result and abort on failure:
- await resetOnForgotPassword(); + const resetOk = await resetOnForgotPassword(); + if (!resetOk) { + setIsBusy(false); + return setError(t('Failed to reset accounts')); + }
82-87: Block navigation when storage operations fail.Lines 82-84 capture storage failures but only log a warning. Navigation and page reload proceed regardless, leaving the application in an inconsistent state where the account exists but critical flags (
SELECTED_PROFILE,IS_PASSWORD_MIGRATED) are not persisted.Apply this diff to prevent navigation on storage failure:
const okProfile = await setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.LOCAL); const okMigrated = await setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true); if (!okProfile || !okMigrated) { - console.warn('Failed to persist profile or migration flag'); + setIsBusy(false); + return setError(t('Failed to save account settings')); } navigate(onSuccessPath) as void; window.location.reload();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (6)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-40)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
resetOnForgotPassword(10-18)packages/extension-polkagate/src/messaging.ts (1)
createAccountSuri(173-175)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/util/constants.ts (2)
STORAGE_KEY(309-330)PROFILE_TAGS(260-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (8)
packages/extension-polkagate/src/hooks/useAutoLock.ts (1)
31-33: LGTM! Memory leak and default fallback issues resolved.The migration to
getAndWatchStoragecorrectly addresses the previously flagged concerns: the unsubscribe function is now captured and returned in the cleanup, preventing listener leaks, and the default value (DEFAULT_AUTO_LOCK) is passed as the fourth parameter, ensuring consistent fallback behavior for both initial load and storage updates.packages/extension-polkagate/src/hooks/useIsForgotten.ts (2)
15-17: LGTM! Storage subscription cleanup properly implemented.The unsubscribe function is correctly captured and returned in the cleanup, resolving the previously flagged memory leak.
12-12: Type annotation mismatch with initial state.The
useStategeneric is typed asForgottenInfo | null, but no initial value is provided, so the actual initial state isundefined. This creates a type inconsistency.Apply this diff to align the type with the actual initial value:
- const [isForgotten, setIsForgotten] = useState<ForgottenInfo | null>(); + const [isForgotten, setIsForgotten] = useState<ForgottenInfo | null | undefined>();Alternatively, initialize explicitly to
null:- const [isForgotten, setIsForgotten] = useState<ForgottenInfo | null>(); + const [isForgotten, setIsForgotten] = useState<ForgottenInfo | null>(null);packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (2)
85-93: Block navigation on storage failures to prevent inconsistent state.This issue was raised in previous reviews but remains unaddressed. If either storage operation fails, the code proceeds with navigation and reload, potentially leaving the application in an inconsistent state where the account exists but critical flags (
SELECTED_PROFILE,IS_PASSWORD_MIGRATED) are not persisted. Additionally,setStoragemay throw rather than returnfalse, which would bypass the current check entirely.Apply this diff to prevent navigation when storage fails:
- const okProfile = await setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.LOCAL); - const okMigrated = await setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true); + const okProfile = await setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.LOCAL).catch(() => false); + const okMigrated = await setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(() => false); if (!okProfile || !okMigrated) { - console.warn('Failed to persist profile or migration flag'); + setIsBusy(false); + return setError(t('Failed to save account settings')); } navigate(onSuccessPath) as void; window.location.reload();
94-97: Surface errors to the user in the catch block.This issue was raised in previous reviews but remains unaddressed. The catch block only logs the error to the console without calling
setError, leaving users unaware that account creation failed.Apply this diff to display the error:
} catch (error) { setIsBusy(false); console.error(error); + setError(t('An error occurred during account creation')); }packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx (3)
95-104: Batch import missestype; align with single-account path.Multi-account mapping does not set the required
type, diverging from the single-account flow andResponseJsonGetAccountInfo. UsejsonGetAccountInfoper item (no password needed) to get canonical fields, then mergeisExternal. Based on snippets.- const accs = json.accounts.map(({ address, meta: { genesisHash, isExternal, name } }) => ({ - address, - genesisHash, - isExternal, - name: name ?? 'Unknown' - } as JsonGetAccountInfo)); - - setAccountsInfo(accs); - setSelectedAccountsInfo(accs); + // Ensure parity with single-account imports: get address/name/genesisHash/type from background + void (async () => { + try { + const infos = await Promise.all(json.accounts.map((a) => jsonGetAccountInfo(a))); + const accs: JsonGetAccountInfo[] = infos.map((info, i) => ({ + ...info, + isExternal: (json.accounts[i].meta as Record<string, unknown>)?.isExternal as boolean | undefined + })); + setAccountsInfo(accs); + setSelectedAccountsInfo(accs); + } catch (e) { + setFileError(true); + console.error(e); + } + })();
174-179: Remove unsafe cast; don’t assumeisExternalexists on context accounts.Casting
maybeExistingAccounts as JsonGetAccountInfo[]is unsafe. Either type‑guard or avoid building a synthetic array when you only need to know if there are any local accounts.- const localAccountsToUnlock = [ - ...selectedAccountsInfo, - ...maybeExistingAccounts as JsonGetAccountInfo[] - ].filter((a, index, self) => - !a.isExternal && self.findIndex((x) => x.address === a.address) === index - ); + // Determine presence of any local (non-external) accounts safely + const hasLocalAccounts = + selectedAccountsInfo.some((a) => !a.isExternal) || + (maybeExistingAccounts || []).some((a: unknown) => + !('isExternal' in (a as Record<string, unknown>)) || !(a as { isExternal?: boolean }).isExternal + );
15-15: UseENDPOINT_TIMEOUT; add fallback navigation; avoid oddas unknown as void.
- Replace manual ms with
ENDPOINT_TIMEOUT.- Navigate to
'/'even when there are no local accounts to unlock.- Prefer
void setStorage(...)(orawait) instead ofas unknown as void.-import { AUTO_LOCK_PERIOD_DEFAULT, PROFILE_TAGS, STORAGE_KEY } from '@polkadot/extension-polkagate/src/util/constants'; +import { ENDPOINT_TIMEOUT, PROFILE_TAGS, STORAGE_KEY } from '@polkadot/extension-polkagate/src/util/constants'; @@ - if (localAccountsToUnlock.length > 0) { - const success = await unlockAllAccounts(password, AUTO_LOCK_PERIOD_DEFAULT * 60 * 1000); + if (hasLocalAccounts) { + const success = await unlockAllAccounts(password, ENDPOINT_TIMEOUT); @@ - if (success) { - setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - navigate('/') as void; - } else { - navigate('/migratePasswords') as void; - } - } + if (success) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true); + navigate('/'); + } else { + navigate('/migratePasswords'); + } + } else { + navigate('/'); + }Also applies to: 181-190
🧹 Nitpick comments (5)
packages/extension-ui/src/Popup/contexts/AccountIconThemeProvider.tsx (1)
18-18: Consider simplifying the cleanup return.The wrapper function is unnecessary since
unsubscribeis already a function.Apply this diff:
- return () => unsubscribe(); + return unsubscribe;packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
15-67: Well-structured component with proper state management.The component correctly wires the auto-lock timer UI with appropriate disabled states, fallback values, and responsive layout. The hook integration is clean, and the conditional logic based on
autoLock?.enabledproperly controls the TextField and DropSelect components.Consider adding explicit min/max constraints for the numeric input to prevent edge cases like zero or unreasonably large values (e.g., 9999 days). While
maxLength={4}limits character count, it doesn't enforce semantic bounds:<MyTextField disabled={!autoLock?.enabled} inputType='number' inputValue={inputValue || autoLock?.delay?.value || AUTO_LOCK_PERIOD_DEFAULT} maxLength={4} + min={1} + max={9999} onTextChange={onDelayValueChange} placeholder='00' style={{ width: isExtension ? '70px' : '88px' }} />This would require verifying that
MyTextFieldsupportsmin/maxprops, or implementing validation in theuseAdjustAutoLockTimerhook.packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (2)
72-72: Fix indentation.Extra leading space before
setIsBusy(false).- if (!resetOk) { - setIsBusy(false); + if (!resetOk) { + setIsBusy(false);
92-93: Consider removing the hard reload if navigate() is sufficient.Calling
window.location.reload()immediately afternavigate()forces a full page reload, which restarts the entire application. This may interrupt the navigation or cause race conditions. Unless there's a specific need to force a reload (e.g., to clear cached state),navigate(onSuccessPath)alone should be sufficient for routing.packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx (1)
171-171: Consider awaiting storage set for deterministic flow (optional).
await setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.ALL);is good here; keep this pattern for the migration flag too if you prefer stronger ordering guarantees before navigation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/extension-polkagate/src/fullscreen/settings/partials/AccountIcon.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/DiagnosticsReports.tsx(1 hunks)packages/extension-polkagate/src/hooks/useAutoLock.ts(2 hunks)packages/extension-polkagate/src/hooks/useIsForgotten.ts(1 hunks)packages/extension-polkagate/src/hooks/useProfileAccounts.ts(2 hunks)packages/extension-polkagate/src/hooks/useSelectedChains.ts(2 hunks)packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)packages/extension-polkagate/src/util/storage.ts(1 hunks)packages/extension-ui/src/Popup/contexts/AccountIconThemeProvider.tsx(1 hunks)packages/extension-ui/src/Popup/contexts/CurrencyProvider.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
packages/extension-polkagate/src/hooks/useProfileAccounts.ts (2)
packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
packages/extension-ui/src/Popup/contexts/CurrencyProvider.tsx (3)
packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)packages/extension-polkagate/src/util/currencyList.ts (1)
USD_CURRENCY(6-11)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (6)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-40)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
resetOnForgotPassword(10-18)packages/extension-polkagate/src/messaging.ts (1)
createAccountSuri(173-175)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/util/constants.ts (2)
STORAGE_KEY(309-330)PROFILE_TAGS(260-266)
packages/extension-ui/src/Popup/contexts/AccountIconThemeProvider.tsx (2)
packages/extension-polkagate/src/util/constants.ts (2)
DEFAULT_ACCOUNT_ICON_THEME(303-303)STORAGE_KEY(309-330)packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (5)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
useAdjustAutoLockTimer(17-78)packages/extension-polkagate/src/util/constants.ts (1)
AUTO_LOCK_PERIOD_DEFAULT(226-226)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts (1)
autoLockOptions(4-8)
packages/extension-polkagate/src/hooks/useAutoLock.ts (2)
packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx (7)
packages/extension-base/src/background/types.ts (1)
ResponseJsonGetAccountInfo(457-462)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-base/src/background/handlers/Extension.ts (1)
jsonGetAccountInfo(430-444)packages/extension-polkagate/src/messaging.ts (2)
jsonGetAccountInfo(271-273)unlockAllAccounts(100-102)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
resetOnForgotPassword(10-18)packages/extension-polkagate/src/util/constants.ts (2)
AUTO_LOCK_PERIOD_DEFAULT(226-226)STORAGE_KEY(309-330)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (3)
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
packages/extension-polkagate/src/hooks/useSelectedChains.ts (2)
packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
packages/extension-polkagate/src/fullscreen/settings/partials/AccountIcon.tsx (2)
packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (2)
STORAGE_KEY(309-330)DEFAULT_ACCOUNT_ICON_THEME(303-303)
packages/extension-polkagate/src/fullscreen/settings/partials/DiagnosticsReports.tsx (2)
packages/extension-polkagate/src/util/storage.ts (1)
getAndWatchStorage(56-77)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
packages/extension-ui/src/Popup/contexts/AccountIconThemeProvider.tsx (1)
9-9: LGTM! Clean refactoring to use unified storage utility.The refactoring consolidates the previous
getStorage+watchStoragepattern into a singlegetAndWatchStoragecall, which simplifies initialization and cleanup logic. Error handling is now centralized in the utility function, and the default value behavior is preserved.Also applies to: 16-16, 18-18
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
27-27: Past issue resolved correctly.The invalid
alignItems='left'values have been properly corrected toalignItems='flex-start', ensuring valid CSS flexbox alignment.Also applies to: 36-36
packages/extension-polkagate/src/util/storage.ts (1)
56-76: LGTM! Well-designed generic storage watcher.The typed
getAndWatchStorage<T>function provides a clean abstraction for initializing and subscribing to storage with default fallback. The logic correctly handlesundefined,null, and empty parsed objects, and the improved error logging includes the storage key for better debugging.packages/extension-polkagate/src/fullscreen/settings/partials/DiagnosticsReports.tsx (1)
19-21: LGTM!Properly captures and cleans up the storage subscription.
packages/extension-ui/src/Popup/contexts/CurrencyProvider.tsx (1)
21-23: LGTM!The migration to
getAndWatchStoragewithUSD_CURRENCYas the default is clean and properly handles cleanup.packages/extension-polkagate/src/fullscreen/settings/partials/AccountIcon.tsx (1)
78-80: LGTM!The migration to
getAndWatchStoragewithDEFAULT_ACCOUNT_ICON_THEMEas the default is correct, and cleanup is properly handled.packages/extension-polkagate/src/hooks/useSelectedChains.ts (1)
19-21: LGTM!The storage subscription is correctly set up with the default selected chains and properly cleaned up on unmount.
packages/extension-polkagate/src/hooks/useProfileAccounts.ts (1)
26-28: LGTM!The storage subscription correctly defaults to
'All'when no explicit profile is provided, and cleanup is properly handled.packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (5)
1-23: LGTM!The imports are appropriate, and the hook setup correctly initializes navigation, translation, and password validation utilities.
25-29: LGTM!State declarations are appropriate for managing the account creation flow.
31-33: LGTM!Clearing the error when the password changes provides good user experience.
35-51: LGTM!The validation callback properly handles both success and failure cases with appropriate error handling.
100-113: LGTM!The return object properly exposes all necessary state and callbacks for consumers of this hook.
Summary by CodeRabbit
New Features
UI/UX
Localization
Chores